Re: [PATCH] affs: Fix resource leaks
On Thu, Feb 03, 2022 at 11:58:42AM +, Darren Kenny wrote: > Hi Alec, > > Don't know how I ended up missing this originally, but thanks for fixing > it, so: > > Reviewed-by: Darren Kenny Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/4] Clean up code and fix coverity bugs in util/grub-module-verifierXX.c
On Thu, Feb 03, 2022 at 11:56:49AM +, Darren Kenny wrote: > Hi Alec, > > These look good to me, thanks for handling the Coverity issues here. > > For the series: > > Reviewed-by: Darren Kenny Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] riscv: adjust -march flags for binutils 2.38
On Sat, Jan 29, 2022 at 01:36:55PM +0100, Heinrich Schuchardt wrote: > As of version 2.38 binutils defaults to ISA specification version > 2019-12-13. This version of the specification has has separated the > the csr read/write (csrr*/csrw*) instructions and the fence.i from > the I extension and put them into separate Zicsr and Zifencei > extensions. > > This implies that we have to adjust the -march flag passed to the > compiler accordingly. > > Signed-off-by: Heinrich Schuchardt Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 3/4] luks2: set up dummy sector size during scan
Hi,
Am Mittwoch, 22. Dezember 2021, 19:17:37 CET schrieb Josselin Poiret via
Grub-devel:
> Hello everyone,
>
> Fabian Vogt writes:
> > It looks like we have a third patch (series) for this feature meanwhile:
> > [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe
> >
> > I CC'd the author, let's try to coordinate.
And there's a forth one now (author CC'd)!
("[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices")
So we have:
"[PATCH 3/4] luks2: set up dummy sector size during scan", which hardcodes 512,
"[PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info of the
cheat device", which queries the sector size of the underlying host device,
"[PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from
DM parameters", which parses the DM table to get the sector_size, and now
"[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices", which changes the
grub core code to accept a sector size of 0 for LUKS2 devices.
Should be enough options to pick a good one ;-)
> > Thanks,
> > Fabian
>
> Let me just say that I had not found this patch series while searching
> beforehand. Let me just recap what my patches do differently (in
> relation to patches 3 and 4 of this series):
>
> Because cheat-mounting cryptodisks only happens (from my understanding)
> when pulling devmapper devices, we can simply ask the kernel for the dm
> and dm-crypt parameters that it's opened with, and populate our
> cheat-mounted device from that. This completely circumvents the multiple
> segments issue, as this will always yield the parameters corresponding
> to the user-specified mountpoint of `grub-probe` or `grub-install`.
Yup.
Did you have a look at my approach? That effectively does the same, but using a
single ioctl instead of anything complex with DM directly.
> I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as
> to reuse all existing code that supports LUKS1, although that can be
> confusing. We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to
> GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how
> they're unlocked, not in underlying algorithms.
>
> What do you think?
Sounds good to me, though I'd count that as a separate refactoring step for
the future.
Cheers,
Fabian
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/2] Support plain encryption mode.
On Wed, 02 Feb 2022 15:00:10 + Maxim Fomin wrote: > --- Original Message --- > > > > > > Plainmount can work with '(hdX,gptY)' syntax in config or shell > > > (actually, this > > > > > > is the base syntax) and thus it is not limited to GPT paritions, what is > > > limited > > > > > > is the ability to use UUID - currently only on GPT. If partition scheme > > > does not > > > > > > have UUID then UUID as a convenience feature cannot be supported - > > > inconvenient, > > > > > > but technically fair. I will take a look at MBR UUID and see whether they > > > can be > > > > > > supported. Possible situations (under current implementaion) are follows: > > > > > > a) GPT disk, multi-disk environment, disks map unpredictably: can name > > > partitions > > > > > > by GPT UUID in config file/shell, no problem, ability to name by UUID has > > > value > > > > I agree that searching by partition UUID is useful and desirable. > > > > However, I don't think this is the right approach. GRUB should have > > > > generic searching by partition UUID. There is already a patch for > > > > this[1]. Perhaps you can test/review this patch to help it gain more > > > > visibility and advocate for it being accepted. > > > > Glenn > > > > [1] https://lists.gnu.org/archive/html/grub-devel/2021-04/msg00055.html > > > > Such function (or several functions) should be added into grub 'library', so > it can be > used to search disk by PART UUID in different places. The patch you refer to > seems to > add this functionality only to 'search' grub command via 'void > grub_search_partuuid' > function. Can it be reused on other places? It seems in oder to use it, grub > code must > call 'search' command and receive the result from grub environment variable > which is > not convinient for other grub code interested in this feature. I think the > proper way > to do it is to write some library function which can be used by search, probe > (btw I > borrowed some details from it - so there is code duplication in search/probe), > plainmount commands and other commands in grub. I agree that reducing code duplication would be a good idea. Essentially the grub 'library' you're wanting exists as the kernel code that is always loaded. Would you like to find a good place to put the common partition uuid matching code and send a patch? As far as plainmount is concerned, I wasn't envisioning that it use that code directly and it shouldn't. I was imagining something like this snippet of GRUB script: search --partuuid --set KEYFILEDISK -u $PARTUUID plainmount -k ($KEYFILEDISK)/path/to/keyfile Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 1/2] plainmount: Support decryption of devices encrypted in plain mode.
On Wed, 02 Feb 2022 14:55:46 +
Maxim Fomin wrote:
> --- Original Message ---
>
> On Tuesday, February 1st, 2022 at 5:30, Glenn Washburn
> wrote:
>
> > On Sun, 30 Jan 2022 19:40:43 +
> >
> > Maxim Fomin ma...@fomin.one wrote:
> >
> > > This patch introduces support for plain encryption mode (plain dm-crypt)
> > > via
> > >
> > > new module and command named 'plainmount'. The command allows to open
> > > devices
> > >
> > > encrypted in plain mode (without LUKS) with following syntax:
> > >
> > > plainmount -h hash -c cipher -o offset -s size -k key-size
> > >
> > > -z sector-size -d keyfile -O keyfile offset -l keyfile-size
> > >
> > > can be some partition or GPT UUID, keyfile can be
> > > /some/path.
> >
> > I'm not in favor of the keyfile UUID prefix for paths. Why not just use
> >
> > the normal device syntax?
>
> Normal syntax can be used, it is base case syntax. Not supporting UUID for
> keyfile (but supporting for device parameter) significantly limits possible
> use cases because keyfile can be located on another drive. For example,
> several linux wiki/manuals present one of several setup scenarios where
> keyfile is located on a separate disk. This makes having support for UUID
> for keyfile is almost surely necessary. However, support for UUID in
> plainmount can be temporarily disabled untill the situation with search
> command patch is resolved.
I think this code should assume that the user/script knows how to get
the full path needed.
>
> > >
> > > +static const struct grub_arg_option options[] =
> > >
> > > - {
> > > - /* TRANSLATORS: It's still restricted to this module only. */
> >
> > It would be nice to allow specifying a password as an argument (-p)
> >
> > like in cryptomount for consistency. It'll allow tests to no need to
> >
> > write a keyfile also.
>
> I agree if this option is alternative way of requesting password and the
> ability to type password remains.
Yes, in addition to. The password will still be prompted if no -k or -p
arguments are present.
>
> >
> > > - {"hash", 'h', 0, N_("Password hash."), 0, ARG_TYPE_STRING},
> > > - {"cipher", 'c', 0, N_("Password cipher."), 0, ARG_TYPE_STRING},
> > > - {"offset", 'o', 0, N_("Device offset (512 bit blocks)."), 0,
> > > ARG_TYPE_INT},
> >
> > s/bit/byte/
> >
> > And why 512-byte blocks? The LUKS2 header stores offset as bytes,
> >
> > perhaps bytes should be used here too.
>
> I followed cryptsetup command syntax for plain mode - it asks offset in terms
> of
> 512 byte segments because it is easier to type such number than number in
> bytes.
> In general offset can be large and also inconvenient to type, but in practice
> offset starts from 1-100 MiB, so this number is easier to remember (why it is
> done this way in cryptsetup is my guess). Anyway, specifying in this way is
> more
> familiar for cryptsetup users, but this can be changed.
I was misremembering that cryptsetup used a number in 512-byte sectors
for those arguments. In the LUKS header is stored in bytes. I like the
multiplicative suffixes that "dd" uses and bytes are assumed if no
suffix. Although it uses 'b' as the suffix for 512-bytes, which seems
non-obvious to me. Parted uses 's' for 512-byte sectors which I like
better.
> > >
> > > +/* Disk iterate callback */
> > >
> > > +static int grub_plainmount_scan_real (const char *name, void *data)
> > >
> > > +{
> > >
> > > - int ret = 0;
> > > - struct grub_plainmount_iterate_args *args = data;
> > > - grub_disk_t source = NULL, disk = NULL;
> > > - struct grub_partition *partition;
> > > - struct grub_gpt_partentry entry;
> > > - grub_gpt_part_guid_t *guid;
> > > - /* UUID format: ---- + '\0' */
> > > - char uuid[37] = "";
> > >
> > > - source = grub_disk_open (name);
> > > - if (!source)
> > > -goto exit;
> > >
> > >
> > > - if (!source->partition)
> > > -goto exit;
> > >
> > >
> > > - partition = source->partition;
> > > - if (grub_strcmp (partition->partmap->name, "gpt") != 0)
> > > -goto exit;
> > >
> > >
> >
> > As noted elsewhere, I'm not in favor of these checks, nor the forcing
> >
> > of the volume to be on a partition.
>
> I didn't consider the case for disk instead of partition, will think about it.
>
> > >
> > >
> > > -default:
> > >
> > >
> > > - grub_error (GRUB_ERR_BAD_ARGUMENT,
> > >
> > >
> > > - N_("invalid sector size -z %"PRIuGRUB_SIZE
> > >
> > >
> > > - ", only 512/1024/2048/4096 are allowed"),
> > >
> > >
> > > - cargs->sector_size);
> > >
> > >
> > > - grub_print_error ();
> > >
> > >
> > > - return GRUB_ERR_BAD_ARGUMENT;
> > >
> > >
> >
> > Should just set the error and return. Further up the call stack the
> >
> > error should be printed. So do something like
> >
> > return grub_error (GRUB_ERR_BAD_ARGUMENT,
> >
> > N_("invalid sector size -z %"PRIuGRUB_SIZE
> >
Re: [PATCH] search: new --efidisk-only option on EFI systems
On Tue, 1 Feb 2022 11:36:01 +0100
Renaud Métrich wrote:
> When using 'search' on EFI systems, we sometimes want to exclude devices
> that are not EFI disks (e.g. md, lvm).
> This is typically used when wanting to chainload when having a software
> raid (md) for EFI partition:
>
> with no option, 'search --file /EFI/redhat/shimx64.efi' sets root envvar
> to 'md/boot_efi' which cannot be used for chainloading since there is no
> effective EFI device behind.
>
> Example of "grub.cfg" file used to chainload when system boots over the
> network:
In the future, please submit patches inline and not attached. Otherwise
it makes it more difficult to respond to its contents. I've added the
patch inline in my response and quoted.
>
> ~~~
>
> menuentry 'Chainload Grub2 EFI from ESP' --id local {
>
> unset root
>
> search --file --no-floppy --efidisk-only --set=root /EFI/BOOT/BOOTX64.EFI
>
> if [ -f ($root)/EFI/BOOT/grubx64.efi ]; then
>
> chainloader ($root)/EFI/BOOT/grubx64.efi
>
> elif [ -f ($root)/EFI/redhat/shimx64.efi ]; then
>
> chainloader ($root)/EFI/redhat/shimx64.efi
>
> elif [ -f ($root)/EFI/redhat/grubx64.efi ]; then
>
> chainloader ($root)/EFI/redhat/grubx64.efi
>
> fi
>
> }
>
> ~~~
>
>
> This patch has been tested on QEMU/KVM systems and VMWare VMs (at
> hardware level 6.7 and 7.0u2).
>
> Related Red Hat BZ (public):
> https://bugzilla.redhat.com/show_bug.cgi?id=2048904
>
> From 46a869395e08fd2df4f722483b8db0f7da62 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Renaud=20M=C3=A9trich?=
> Date: Tue, 1 Feb 2022 07:17:24 +0100
> Subject: [PATCH] search: new --efidisk-only option on EFI systems
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> When using 'search' on EFI systems, we sometimes want to exclude
> devices that are not EFI disks (e.g. md, lvm).
> This is typically used when wanting to chainload when having a
> software raid (md) for EFI partition:
> with no option, 'search --file /EFI/redhat/shimx64.efi' sets root
> envvar to 'md/boot_efi' which cannot be used for chainloading since
> there is no effective EFI device behind.
>
> Signed-off-by: Renaud Métrich
>
> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> index ed090b3af..5e1e88643 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -48,6 +48,9 @@ struct search_ctx
>const char *key;
>const char *var;
>int no_floppy;
> +#ifdef GRUB_MACHINE_EFI
> + int efidisk_only;
> +#endif
I think it would be cleaner to have a "flags" member here where right
now there would only be SEARCH_FLAGS_NO_FLOPPY and
SEARCH_FLAGS_EFIDISK_ONLY. This way if in the future someone wants to
add another filter to search they just add a flag instead of updating
all the function signatures. And for this patch it will remove the need
for most of the #ifdefs.
>char **hints;
>unsigned nhints;
>int count;
> @@ -64,7 +67,28 @@ iterate_device (const char *name, void *data)
>/* Skip floppy drives when requested. */
>if (ctx->no_floppy &&
>name[0] == 'f' && name[1] == 'd' && name[2] >= '0' && name[2]
> <= '9')
> -return 1;
> +return 0;
So this function now returns success if --no-floppy was requested and
we've encountered a floppy? Am I missing something? If this is
desirable, perhaps it should be documented.
Glenn
> +
> +#ifdef GRUB_MACHINE_EFI
> + /* Limit to EFI disks when requested. */
> + if (ctx->efidisk_only)
> +{
> + grub_device_t dev;
> + dev = grub_device_open (name);
> + if (! dev)
> + {
> + grub_errno = GRUB_ERR_NONE;
> + return 0;
> + }
> + if (! dev->disk || dev->disk->dev->id !=
> GRUB_DISK_DEVICE_EFIDISK_ID)
> + {
> + grub_device_close (dev);
> + grub_errno = GRUB_ERR_NONE;
> + return 0;
> + }
> + grub_device_close (dev);
> +}
> +#endif
>
> #ifdef DO_SEARCH_FS_UUID
> #define compare_fn grub_strcasecmp
> @@ -262,12 +286,18 @@ try (struct search_ctx *ctx)
>
> void
> FUNC_NAME (const char *key, const char *var, int no_floppy,
> +#ifdef GRUB_MACHINE_EFI
> +int efidisk_only,
> +#endif
> char **hints, unsigned nhints)
> {
>struct search_ctx ctx = {
> .key = key,
> .var = var,
> .no_floppy = no_floppy,
> +#ifdef GRUB_MACHINE_EFI
> +.efidisk_only = efidisk_only,
> +#endif
> .hints = hints,
> .nhints = nhints,
> .count = 0,
> @@ -303,8 +333,12 @@ grub_cmd_do_search (grub_command_t cmd
> __attribute__ ((unused)), int argc, if (argc == 0)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument
> expected"));
> - FUNC_NAME (args[0], argc == 1 ? 0 : args[1], 0, (args + 2),
> - argc > 2 ? argc - 2 : 0);
> + FUNC_NAME (args[0], argc == 1 ? 0 : args[1], 0,
> +#ifdef GRUB_MACHINE_EFI
> + /* efidisk_only */
> + 0,
> +#endif
> + (args + 2), argc > 2 ? argc - 2 : 0);
>
