499 Commits

Author SHA1 Message Date
Oliver Steffen
06edd40db7 guid: Unify GUID types
There are 3 implementations of a GUID in GRUB. Replace them with
a common one, placed in types.h.

It uses the "packed" flavor of the GUID structs, the alignment attribute
is dropped, since it is not required.

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2023-06-01 11:45:00 +02:00
Ard Biesheuvel
bb4aa6e06e efi: Drop all uses of efi_call_XX() wrappers
Now that GCC can generate function calls using the correct calling
convention for us, we can stop using the efi_call_XX() wrappers, and
just dereference the function pointers directly.

This avoids the untyped variadic wrapper routines, which means better
type checking for the method calls.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2023-05-25 16:48:00 +02:00
Fabian Vogt
efc9c363b2 disk/cryptodisk: When cheatmounting, use the sector info of the cheat device
When using grub-probe with cryptodisk, the mapped block device from the host
is used directly instead of decrypting the source device in GRUB code.
In that case, the sector size and count of the host device needs to be used.
This is especially important when using LUKS2, which does not assign
total_sectors and log_sector_size when scanning, but only later when the
segments in the JSON area are evaluated. With an unset log_sector_size,
grub_device_open() complains.

This fixes grub-probe failing with
"error: sector sizes of 1 bytes aren't supported yet.".

Signed-off-by: Fabian Vogt <fvogt@suse.de>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Tested-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2023-02-01 16:16:54 +01:00
Glenn Washburn
9e652cb6f2 misc: Fix spacing
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2023-01-19 17:39:04 +01:00
Maxim Fomin
5464e31a4e disk/plainmount: Support plain encryption mode
This patch adds support for plain encryption mode, plain dm-crypt, via
new module/command named "plainmount".

Signed-off-by: Maxim Fomin <maxim@fomin.one>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Glenn Washburn <development@efficientek.com>
2023-01-10 15:59:51 +01:00
Daniel Axtens
2a5e3c1f2a disk/diskfilter: Don't make a RAID array with more than 1024 disks
This is "belt and braces" with commit 12e20a6a695f (disk/diskfilter:
Check calloc() result for NULL): we end up trying to use too much memory
in situations like corrupted Linux software RAID setups purporting to
use a huge number of disks. Simply refuse to permit such configurations.

1024 is a bit arbitrary, yes, and I feel a bit like I'm tempting fate
here, but I think 1024 disks in an array (that GRUB has to read to boot!)
should be enough for anyone.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-10-27 20:10:18 +02:00
Alec Brown
c76a07e15f disk/cryptodisk: Fix unintentional integer overflow
In the function grub_cryptodisk_endecrypt(), a for loop is incrementing the
variable i by (1U << log_sector_size). The variable i is of type grub_size_t
which is a 64-bit unsigned integer on x86_64 architecture. On the other hand, 1U
is a 32-bit unsigned integer. By performing a left shift on a 32-bit value and
assigning it to a 64-bit variable, the 64-bit variable may have incorrect values
in the high 32-bits if the shift has an overflow. To avoid this, we replace 1U
with (grub_size_t)1.

Fixes: CID 307788

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-10-27 15:57:14 +02:00
Daniel Axtens
12e20a6a69 disk/diskfilter: Check calloc() result for NULL
With wildly corrupt inputs, we can end up trying to calloc a very
large amount of memory, which will fail and give us a NULL pointer.
We need to check that to avoid a crash. (And, even if we blocked
such inputs, it is good practice to check the results of allocations
anyway.)

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-10-11 14:49:39 +02:00
Glenn Washburn
3cf2e848bc disk/cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner
A user can now specify UUID strings with dashes, instead of having to remove
dashes. This is backwards-compatibility preserving and also fixes a source
of user confusion over the inconsistency with how UUIDs are specified
between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
reference implementation for LUKS, displays and generates UUIDs with dashes
there has been additional confusion when using the UUID strings from
cryptsetup as exact input into GRUB does not find the expected cryptodisk.

A new function grub_uuidcasecmp() is added that is general enough to be used
other places where UUIDs are being compared.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-10-11 14:42:07 +02:00
Glenn Washburn
084dfe6d9c disk/loopback: Support transparent decompression of backing file
A new option is added to the loopback command, -D or --decompress, which
when specified transparently decompresses the backing file. This allows
compressed images to be used as if they were uncompressed.

Add documentation to support this change.

Suggested-by: Li Gen <ligenlive@gmail.com>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-10-04 15:45:30 +02:00
Patrick Steinhardt
de6921ced3 disk/luks2: Fix decoding of digests and salts with escaped chars
It was reported in the #grub IRC channel on Libera that decryption of
LUKS2 partitions fails with errors about invalid digests and/or salts.
In all of these cases, what failed was decoding the Base64
representation of these, where the encoded data contained invalid
characters.

As it turns out, the root cause is that json-c, which is used by
cryptsetup to read and write the JSON header, will escape some
characters by prepending a backslash when writing JSON strings by
default. Most importantly, json-c also escapes the forward slash, which
is part of the Base64 alphabet. Because GRUB doesn't know to unescape
such characters, decoding this string will rightfully fail.

Interestingly, this issue has until now only been reported by users of
Ubuntu 18.04. And a bit of digging in fact reveals that cryptsetup has
changed the logic in a054206d (Suppress useless slash escaping in json
lib, 2018-04-20), which has been released with cryptsetup v2.0.3. Ubuntu
18.04 is still shipping with cryptsetup v2.0.2 though, which explains
why this is not a more frequent issue.

Fix the issue by using our new grub_json_unescape() helper function
that handles unescaping for us.

Reported-by: Afdal
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <dkiper@net-space.pl>
2022-08-19 23:55:50 +02:00
Glenn Washburn
0b44244025 disk/cryptodisk: Support encrypted volumes using detached headers on a partition
Update the read hook to take into account encrypted volumes on a partition.
GRUB disk read hooks supply an absolute sector number at which the read is
started from. If the encrypted volume is in a partition, the sector number
given to the read hook will be offset by the number of the sector at the
start of the partition. The read hook then needs to subtract the partition
start from the supplied sector to get the correct start sector for the read
into the detached header file.

Reported-by: brutser <brutser@perso.be>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Tested-by: brutser <brutser@perso.be>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-08-19 20:49:31 +02:00
Glenn Washburn
a768876c0d disk/luks2: Continue trying all keyslots even if there are some failures
luks2_get_keyslot() can fail for a variety of reasons that do not necessarily
mean the next keyslot should not be tried (e.g. a new kdf type). So always
try the next slot. This will make GRUB more resilient to non-spec json data
that 3rd party systems may add. We do not care if some of the keyslots are
unusable, only if there is at least one that is.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-08-10 14:24:46 +02:00
Glenn Washburn
351c9c2fd0 disk: Replace code that calculates the log of sector size with grub_log2ull()
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-07-04 14:43:26 +02:00
Glenn Washburn
1deb521452 cryptodisk: Add support for using detached header files
Using the disk read hook mechanism, setup a read hook on the source disk
which will read from the given header file during the scan and recovery
cryptodisk backend functions. Disk read hooks are executed after the data
has been read from the disk. This is okay, because the read hook is given
the read buffer before its sent back to the caller. In this case, the hook
can then overwrite the data read from the disk device with data from the
header file sent in as the read hook data. This is transparent to the
read caller. Since the callers of this function have just opened the
source disk, there are no current read hooks, so there's no need to
save/restore them nor consider if they should be called or not.

This hook assumes that the header is at the start of the volume, which
is not the case for some formats (e.g. GELI). So GELI will return an
error if a detached header is specified. It also can only be used
with formats where the detached header file can be written to the
first blocks of the volume and the volume could still be unlocked.
So the header file can not be formatted differently from the on-disk
header. If these assumpts are not met, detached header file processing
must be specially handled in the cryptodisk backend module.

The hook will be called potentially many times by a backend. This is fine
because of the assumptions mentioned and the read hook reads from absolute
offsets and is stateless.

Also add a --header (short -H) option to cryptomount which takes a file
argument.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-07-04 14:43:25 +02:00
Stefan Agner
858a0745c8 disk/efi/efidisk: Pass buffers with higher alignment
Some devices report IoAlign values but seem to require buffers with
higher alignment.

The UEFI specification is saying: "IoAlign values of 0 and 1 mean that
the buffer can be placed anywhere in memory. Otherwise, IoAlign must
be a power of 2, and the requirement is that the start address of
a buffer must be evenly divisible by IoAlign with no remainder."

Some devices report IoAlign of 2, however seem to require 4 bytes
aligned buffers. It seems that this got misinterpreted by some vendors
assuming IoAlign is 2^IoAlign. There is also such a hint in an example
in earlier versions of the Driver Writer's Guide:

  ScsiPassThruMode.IoAlign = 2; // Data must be alligned on 4-byte boundary

Some devices report no alignment requirements at all but seem to read
corrupted data or report read errors when passing unaligned buffers.

Work around by using an alignment of at least BlockSize (typically 512
bytes) in any case. If IoAlign (interpreted as per UEFI specification)
requests a higher alignment than BlockSize, follow IoAlign still.

Note: The problem has only noticed with compressed squashfs. It seems
that ext4 (and presumably other file system drivers) pass buffers with
a higher alignment already.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canaonical.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-06-07 13:40:48 +02:00
Glenn Washburn
af81ecede1 disk/cryptodisk: Use enum constants as indexes into cryptomount option array
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-06-07 13:24:54 +02:00
John Lane
81b2f625f5 disk/cryptodisk: Add options to cryptomount to support keyfiles
Add the options --key-file, --keyfile-offset, and --keyfile-size to
cryptomount and code to put read the requested key file data and pass
via the cargs struct. Note, key file data is for all intents and purposes
equivalent to a password given to cryptomount. So there is no need to
enable support for key files in the various crypto backends (e.g. LUKS1)
because the key data is passed just as if it were a password.

Signed-off-by: John Lane <john@lane.uk.net>
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-06-07 13:22:14 +02:00
Denis 'GNUtoo' Carikli
100e410e9c disk/geli: Unify grub_cryptodisk_dev function names
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-06-07 13:06:38 +02:00
Denis 'GNUtoo' Carikli
6fe6229638 disk/luks: Unify grub_cryptodisk_dev function names
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-06-07 13:04:17 +02:00
Michael Chang
acffb81485 build: Fix -Werror=array-bounds array subscript 0 is outside array bounds
The GRUB is failing to build with GCC-12 in many places like this:

  In function 'init_cbfsdisk',
      inlined from 'grub_mod_init' at ../../grub-core/fs/cbfs.c:391:3:
  ../../grub-core/fs/cbfs.c:345:7: error: array subscript 0 is outside array bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
    345 |   ptr = *(grub_uint32_t *) 0xfffffffc;
        |   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This is caused by GCC regression in 11/12 [1]. In a nut shell, the
warning is about detected invalid accesses at non-zero offsets to NULL
pointers. Since hardwired constant address is treated as NULL plus an
offset in the same underlying code, the warning is therefore triggered.

Instead of inserting #pragma all over the places where literal pointers
are accessed to avoid diagnosing array-bounds, we can try to borrow the
idea from Linux kernel that the absolute_pointer() macro [2][3] is used
to disconnect a pointer using literal address from it's original object,
hence GCC won't be able to make assumptions on the boundary while doing
pointer arithmetic. With that we can greatly reduce the code we have to
cover up by making initial literal pointer assignment to use the new
wrapper but not having to track everywhere literal pointers are
accessed. This also makes code looks cleaner.

Please note the grub_absolute_pointer() macro requires to be invoked in
a function as long as it is compound expression. Some global variables
with literal pointers has been changed to local ones in order to use
grub_absolute_pointer() to initialize it. The shuffling is basically done
in a selective and careful way that the variable's scope doesn't matter
being local or global, for example, the global variable must not get
modified at run time throughout. For the record, here's the list of
global variables got shuffled in this patch:

  grub-core/commands/i386/pc/drivemap.c:int13slot
  grub-core/term/i386/pc/console.c:bios_data_area
  grub-core/term/ns8250.c:serial_hw_io_addr

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
[2] https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180
[3] https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31

Signed-off-by: Michael Chang <mchang@suse.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-04-20 18:27:52 +02:00
Robbie Harwood
49b52b4d87 gnulib: Handle warnings introduced by updated gnulib
- Fix type of size variable in luks2_verify_key()
- Avoid redefinition of SIZE_MAX and ATTRIBUTE_ERROR
- Work around gnulib's int types on older compilers

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-03-21 19:17:50 +01:00
Robbie Harwood
2cd52835fc config: Where present, ensure config-util.h precedes config.h
gnulib defines go in config-util.h, and we need to know whether to
provide duplicates in config.h or not.

Signed-off-by: Robbie Harwood <rharwood@redhat.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-03-21 18:47:16 +01:00
Elyes Haouas
b441ca3238 disk: Remove trailing whitespaces
Signed-off-by: Elyes Haouas <ehaouas@noos.fr>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-03-14 15:44:45 +01:00
Glenn Washburn
980cffdbb0 cryptodisk: Fix Coverity use after free bug
The Coverity output is:

  *** CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  /grub-core/disk/cryptodisk.c: 1064 in grub_cryptodisk_scan_device_real()
  1058      cleanup:
  1059       if (askpass)
  1060         {
  1061           cargs->key_len = 0;
  1062           grub_free (cargs->key_data);
  1063         }
  >>>     CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  >>>     Using freed pointer "dev".
  1064       return dev;
  1065     }
  1066
  1067     #ifdef GRUB_UTIL
  1068     #include <grub/util/misc.h>
  1069     grub_err_t

Here the "dev" variable can point to a freed cryptodisk device if the
function grub_cryptodisk_insert() fails. This can happen only on a OOM
condition, but when this happens grub_cryptodisk_insert() calls grub_free on
the passed device. Since grub_cryptodisk_scan_device_real() assumes that
grub_cryptodisk_insert() is always successful, it will return the device,
though the device was freed.

Change grub_cryptodisk_insert() to not free the passed device on failure.
Then on grub_cryptodisk_insert() failure, free the device pointer. This is
done by going to the label "error", which will call cryptodisk_close() to
free the device and set the device pointer to NULL, so that a pointer to
freed memory is not returned.

Fixes: CID 366905

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2022-02-07 20:15:26 +01:00
Glenn Washburn
de14c4b36d cryptodisk: Improve handling of partition name in cryptomount password prompt
Call grub_partition_get_name() unconditionally to initialize the part
variable. Then part will only be NULL when grub_partition_get_name() errors.
Note that when source->partition is NULL, then grub_partition_get_name()
returns an allocated empty string. So no comma or partition will be printed,
as desired.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:35:57 +01:00
Glenn Washburn
ebdd82b00c cryptodisk: Move global variables into grub_cryptomount_args struct
Note that cargs.search_uuid does not need to be initialized in various parts
of the cryptomount argument parsing, just once when cargs is declared with
a struct initializer. The previous code used a global variable which would
retain the value across cryptomount invocations.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:31:44 +01:00
Glenn Washburn
ba9fb5d721 cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
The crypto device modules should only be setting up the crypto devices and
not getting user input. This has the added benefit of simplifying the code
such that three essentially duplicate pieces of code are merged into one.

Add documentation of passphrase option for cryptomount as it is now usable.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:16:33 +01:00
Glenn Washburn
be62f0836c cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
Previously, the cryptomount arguments were passed by global variable and
function call argument, neither of which are ideal. This change passes data
via a grub_cryptomount_args struct, which can be added to over time as
opposed to continually adding arguments to the cryptodisk scan and
recover_key.

As an example, passing a password as a cryptomount argument is implemented.
However, the backends are not implemented, so testing this will return a not
implemented error.

Also, add comments to cryptomount argument parsing to make it more obvious
which argument states are being handled.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:08:17 +01:00
Glenn Washburn
e1d22da580 cryptodisk: Improve cryptomount -u error message
When a cryptmount is specified with a UUID, but no cryptodisk backends find
a disk with that UUID, return a more detailed message giving telling the
user that they might not have a needed cryptobackend module loaded.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:05:57 +01:00
Glenn Washburn
f3d6090319 cryptodisk: Improve error messaging in cryptomount invocations
Update such that "cryptomount -u UUID" will not print two error messages
when an invalid passphrase is given and the most relevant error message
will be displayed.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:04:33 +01:00
Glenn Washburn
cabc36ce56 cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded
This displays an error notifying the user that they'll want to load
a backend module to make cryptomount useful.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 02:00:33 +01:00
Glenn Washburn
26406d8908 cryptodisk: Refactor to discard have_it global
The global "have_it" was never used by the crypto-backends, but was used to
determine if a crypto-backend successfully mounted a cryptodisk with a given
UUID. This is not needed however, because grub_device_iterate() will return
1 if and only if grub_cryptodisk_scan_device() returns 1. And
grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
been specified and a cryptodisk was successfully setup by a crypto-backend or
a cryptodisk of the requested UUID is already open.

To implement this grub_cryptodisk_scan_device_real() is modified to return
a cryptodisk or NULL on failure and having the appropriate grub_errno set to
indicated failure. Note that grub_cryptodisk_scan_device_real() will fail now
with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
modules succeed in identifying the source disk.

With this change grub_device_iterate() will return 1 when a crypto device is
successfully decrypted or when the source device has already been successfully
opened. Prior to this change, trying to mount an already successfully opened
device would trigger an error with the message "no such cryptodisk found",
which is at best misleading. The mount should silently succeed in this case,
which is what happens with this patch.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 01:53:43 +01:00
Glenn Washburn
74101d6c99 luks2: Add debug message to align with luks and geli modules
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-12-23 01:52:26 +01:00
Alec Brown
971dd6599d disk/ldm: Fix resource leak
Commit 23e39f50ca7a (disk/ldm: Make sure comp data is freed before exiting from
make_vg()) fixed several spots in make_vg() where comp data was leaking memory
when an error was being handled but missed one. To avoid leaking memory, comp
should be freed when an error is being handled after comp has been successfully
allocated memory in the for loop.

Fixes: 23e39f50ca7a (disk/ldm: Make sure comp data is freed before exiting from make_vg())
Fixes: CID 73804

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-11-22 16:09:57 +01:00
Michael Chang
f3a121eeeb disk/diskfilter: Use nodes in logical volume's segment as member device
Currently the grub_diskfilter_memberlist() function returns all physical
volumes added to a volume group to which a logical volume (LV) belongs.
However, this is suboptimal as it doesn't fit the intended behavior of
returning underlying devices that make up the LV. To give a clear
picture, the result should be identical to running commands below to
display the logical volumes with underlying physical volumes in use.

  localhost:~ # lvs -o lv_name,vg_name,devices /dev/system/root
    LV   VG     Devices
    root system /dev/vda2(512)

  localhost:~ # lvdisplay --maps /dev/system/root
    --- Logical volume ---
      ...
    --- Segments ---
    Logical extents 0 to 4604:
      Type                linear
      Physical volume     /dev/vda2
      Physical extents    512 to 5116

As shown above, we can know system-root LV uses only /dev/vda2 to
allocate it's extents, or we can say that /dev/vda2 is the member device
comprising the system-root LV.

It is important to be precise on the member devices, because that helps
to avoid pulling in excessive dependency. Let's use an example to
demonstrate why it is needed.

  localhost:~ # findmnt /
  TARGET SOURCE                  FSTYPE OPTIONS
  /      /dev/mapper/system-root ext4   rw,relatime

  localhost:~ # pvs
    PV               VG     Fmt  Attr PSize    PFree
    /dev/mapper/data system lvm2 a--  1020.00m    0
    /dev/vda2        system lvm2 a--    19.99g    0

  localhost:~ # cryptsetup status /dev/mapper/data
  /dev/mapper/data is active and is in use.
    type:    LUKS1
    cipher:  aes-xts-plain64
    keysize: 512 bits
    key location: dm-crypt
    device:  /dev/vdb
    sector size:  512
    offset:  4096 sectors
    size:    2093056 sectors
    mode:    read/write

  localhost:~ # vgs
    VG     #PV #LV #SN Attr   VSize  VFree
    system   2   3   0 wz--n- 20.98g    0

  localhost:~ # lvs -o lv_name,vg_name,devices
    LV   VG     Devices
    data system /dev/mapper/data(0)
    root system /dev/vda2(512)
    swap system /dev/vda2(0)

We can learn from above that /dev/mapper/data is an encrypted volume and
also gets assigned to volume group "system" as one of it's physical
volumes. And also it is not used by root device, /dev/mapper/system-root,
for allocating extents, so it shouldn't be taking part in the process of
setting up GRUB to access root device.

However, running grub-install reports error as volume group "system"
contains encrypted volume.

  error: attempt to install to encrypted disk without cryptodisk
  enabled. Set `GRUB_ENABLE_CRYPTODISK=y' in file `/etc/default/grub'.

Certainly we can enable GRUB_ENABLE_CRYPTODISK=y and move on, but that
is not always acceptable since the server may need to be booted unattended.
Additionally, typing passphrase for every system startup can be a big
hassle of which most users would like to avoid.

This patch solves the problem by returning exact physical volume, /dev/vda2,
rightly used by system-root from the example above, thus grub-install will
not error out because the excessive encrypted device to boot the root device
is not configured.

Signed-off-by: Michael Chang <mchang@suse.com>
Tested-by: Olav Reinert <seroton10@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-10-04 15:06:36 +02:00
Miguel Ángel Arruga Vivas
837fe48deb i18n: Format large integers before the translation message
The GNU gettext only supports the ISO C99 macros for integral
types. If there is a need to use unsupported formatting macros,
e.g. PRIuGRUB_UINT64_T, according to [1] the number to a string
conversion should be separated from the code printing message
requiring the internationalization. So, the function grub_snprintf()
is used to print the numeric values to an intermediate buffer and
the internationalized message contains a string format directive.

[1] https://www.gnu.org/software/gettext/manual/html_node/Preparing-Strings.html#No-string-concatenation

Signed-off-by: Miguel Ángel Arruga Vivas <rosen644835@gmail.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-04-13 17:16:04 +02:00
Glenn Washburn
39cfb3eb5c style: Format string macro should have a space between quotes
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-10 15:23:34 +01:00
Glenn Washburn
c95ec30d48 disk/ata: Use format code PRIxGRUB_UINT64_T for 64-bit uint argument in grub_error()
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-10 15:01:08 +01:00
Glenn Washburn
e96c7645f4 grub_error: Use format code PRIuGRUB_SIZE for variables of type grub_size_t
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-10 14:50:44 +01:00
Glenn Washburn
334c0ef3d0 disk/dmraid_nvidia: Format string error in grub_error()
The grub_error() has a format string expecting two arguments, but only one
provided. According to the comments in the struct grub_nv_super definition,
the version field looks like a version number where major.minor is encoded
as each a byte in the two-byte short.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-10 14:50:44 +01:00
Glenn Washburn
308b0495e1 disk/pata: Suppress error message "no device connected"
This error message comes from the grub_print_error() in
grub_pata_device_initialize(), which does not pass on the error, and is
raised in check_device(). The function check_device() needs to return this
as an error because check_device() is also used in grub_pata_open(), which
does pass on this error to indicate that the device can not be used.

This is actually not an error when displayed by grub_pata_device_initialize()
because it just indicates that there are no pata devices seen. This may be
confusing to end users who do not have pata devices yet are loading the
pata module (perhaps implicitly via nativedisk). This also causes unnecessary
output which may need to be accounted for in functional testing.

Instead print to the debug log when check_device() raises this "error" and
pop the error from the error stack. If there is another error on the stack
then print the error stack as those should be real errors.

Signed-off-by: Glenn Washburn <development@efficientek.com>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-10 13:22:45 +01:00
Daniel Axtens
e18a000738 disk/lvm: Do not allow a LV to be it's own segment's node's LV
This prevents infinite recursion in the diskfilter verification code.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:19 +01:00
Daniel Axtens
7012936847 disk/lvm: Sanitize rlocn->offset to prevent wild read
rlocn->offset is read directly from disk and added to the metadatabuf
pointer to create a pointer to a block of metadata. It's a 64-bit
quantity so as long as you don't overflow you can set subsequent
pointers to point anywhere in memory.

Require that rlocn->offset fits within the metadata buffer size.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:19 +01:00
Daniel Axtens
1155d7dffd disk/lvm: Do not overread metadata
We could reach the end of valid metadata and not realize, leading to
some buffer overreads. Check if we have reached the end and bail.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:18 +01:00
Daniel Axtens
db29073fc7 disk/lvm: Do not crash if an expected string is not found
Clean up a bunch of cases where we could have strstr() fail and lead to
us dereferencing NULL.

We'll still leak memory in some cases (loops don't clean up allocations
from earlier iterations if a later iteration fails) but at least we're
not crashing.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:18 +01:00
Daniel Axtens
2958695c4c disk/lvm: Bail on missing PV list
There's an if block for the presence of "physical_volumes {", but if
that block is absent, then p remains NULL and a NULL-deref will result
when looking for logical volumes.

It doesn't seem like LVM makes sense without physical volumes, so error
out rather than crashing.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:18 +01:00
Daniel Axtens
27a79bf38e disk/lvm: Don't blast past the end of the circular metadata buffer
This catches at least some OOB reads, and it's possible I suppose that
if 2 * mda_size is less than GRUB_LVM_MDA_HEADER_SIZE it might catch some
OOB writes too (although that hasn't showed up as a crash in fuzzing yet).

It's a bit ugly and I'd appreciate better suggestions.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:18 +01:00
Daniel Axtens
a8cc95de74 disk/lvm: Don't go beyond the end of the data we read from disk
We unconditionally trusted offset_xl from the LVM label header, even if
it told us that the PV header/disk locations were way off past the end
of the data we read from disk.

Require that the offset be sane, fixing an OOB read and crash.

Fixes: CID 314367, CID 314371

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:18 +01:00
Darren Kenny
a201ad17ca disk/cryptodisk: Fix potential integer overflow
The encrypt and decrypt functions expect a grub_size_t. So, we need to
ensure that the constant bit shift is using grub_size_t rather than
unsigned int when it is performing the shift.

Fixes: CID 307788

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
2021-03-02 15:54:16 +01:00