Re: [PATCH] grub-shell: Put all generated files into working dir and use better file names
On Mon, Aug 22, 2022 at 06:19:02PM -0500, Glenn Washburn wrote: > When running tests there are many invocations of grub-shell, and because the > output files are all random names in the same tmp directory, it becomes more > work to figure out which files went with which grub-shell invocations. So > all generated files from one invocation of grub-shell are put into a > randomly named directory, so as not to collide with other grub-shell > invocations. And now that the generated files can be put in a location where > they will not get stepped on, and they can be named sensible names. > > Signed-off-by: Glenn Washburn Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH 0/6] Dynamic allocation of memory regions and IBM vTPM v2
On Thu, Dec 01, 2022 at 03:47:30PM +0100, Daniel Kiper wrote: > What do you mean by "supported"? It is build tested and, AIUI, all "make > check" > tests pass. Though I am not sure anybody uses this kinda weird target. Well, > I think I knew but I forgot... :-) I thought OLPC used openfirware on i386. Are those things still around? -- Len Sorensen ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [programmer11...@programist.ru: Bug#1021846: grub-install is broken since 2.06-3: error: unknown filesystem]
Steve McIntyre writes: > Hi all! > > программист некто (in CC) reported this bug a few weeks back in > Debian. Since I applied the bundle of filesystem bounds-checking fixes > a few months back, he can't run grub-install. He's done the work to > determine that the patch that breaks things for him is > > 2d014248d540c7e087934a94b6e7a2aa7fc2c704 fs/f2fs: Do not read past the end of > nat journal entries > > The full thread of our discussion is at https://bugs.debian.org/1021846 > > I don't have any knowledge of f2fs to go any further here. Help please! :-) Ergh, apologies for the regression. [somewhat off-topic: The fix came from a crash derived from fuzzing. I am not really knowledgeable about f2fs either - I was just trying to do my best based on what we could derive from the existing driver. In general, filesystems are a nightmare for fuzzing fixes because testing beyond the (quite decent!) tests that the grub test-suite runs is very challenging. There is usually no-one who is both involved in grub security and an expert on any given file system either. We do the best we can. Sadly our regression rate has been climbing, so we may need to come up with some other way to secure file systems or get access to sufficient expertise in the future.] I had a massive, massive work-in-progress spiel where I looked at this code and compared the linux code and counted sizes and so on and so forth. I was getting nowhere. But eventually I realised I had just made an off-by-one error in the test. You're allowed to have up to n = NAT_JOURNAL_ENTRIES entries _inclusive_, because the loop below uses i < n, not i <= n. D'oh. Please try the following: diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c index df6beb544cbd..855e24618c2b 100644 --- a/grub-core/fs/f2fs.c +++ b/grub-core/fs/f2fs.c @@ -650,7 +650,7 @@ get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid, grub_uint16_t n = grub_le_to_cpu16 (data->nat_j.n_nats); grub_uint16_t i; - if (n >= NAT_JOURNAL_ENTRIES) + if (n > NAT_JOURNAL_ENTRIES) return grub_error (GRUB_ERR_BAD_FS, "invalid number of nat journal entries"); If for some reason that doesn't work, please add the following debug code and report the results: diff --git a/grub-core/fs/f2fs.c b/grub-core/fs/f2fs.c index 855e24618c2b..6e49a6d17b7a 100644 --- a/grub-core/fs/f2fs.c +++ b/grub-core/fs/f2fs.c @@ -643,6 +643,10 @@ get_nat_journal (struct grub_f2fs_data *data) return err; } +#ifdef GRUB_UTIL +#include +#endif + static grub_err_t get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid, grub_uint32_t *blkaddr) @@ -650,6 +654,10 @@ get_blkaddr_from_nat_journal (struct grub_f2fs_data *data, grub_uint32_t nid, grub_uint16_t n = grub_le_to_cpu16 (data->nat_j.n_nats); grub_uint16_t i; +#ifdef GRUB_UTIL + fprintf(stderr, "%s: n = %hu\n", __func__, n); +#endif + if (n > NAT_JOURNAL_ENTRIES) return grub_error (GRUB_ERR_BAD_FS, "invalid number of nat journal entries"); Amusingly the debug code shows that the grub-fs-tester tests always have n = 0, which makes sense for a test that doesn't really stress the file-system, and also explains why we didn't catch the bug when it was introduced. Kind regards, Daniel > > - Forwarded message from программист некто > - > > From: программист некто > To: sub...@bugs.debian.org > Date: Sat, 15 Oct 2022 23:54:36 +0300 > Subject: Bug#1021846: grub-install is broken since 2.06-3: error: unknown > filesystem > Message-Id: <3168731665867...@wf4nrjvtssjecb53.iva.yp-c.yandex.net> > > Package: grub-pc > Version: 2.06-3~deb11u1 > Severity: critical > > Hello. Since version 2.06-3, grub-install is broken: it fails with "error: > unknown filesystem". > I test command /usr/sbin/grub-install -v /dev/sda > in some versions. Results in mail attachments. > Versions older than 2.06-3 works without error (2.06-2 and lower). > Tested versions: 2.04-20, 2.06-1, 2.06-2, 2.06-3~deb10u1, 2.06-3~deb11u1, > 2.06-4. > > Disk partitions: > > # fdisk --list-details > Disk /dev/sda: 29,82 GiB, 32017047552 bytes, 62533296 sectors > Disk model: TS32GSSD370S > Units: sectors of 1 * 512 = 512 bytes > Sector size (logical/physical): 512 bytes / 512 bytes > I/O size (minimum/optimal): 512 bytes / 512 bytes > Disklabel type: dos > Disk identifier: 0xc7177f7e > > Device Boot Start End Sectors Id Type Start-C/H/S End-C/H/S Attrs > /dev/sda1 2048 22763519 22761472 83 Linux 4/4/1 1023/254/2 > /dev/sda2 * 25866240 62531583 36665344 7 HPFS/ 1023/254/2 1023/254/2 80 > > $ disktype /dev/sda1 > --- /dev/sda1 > Block device, size 10.85 GiB (11653873664 bytes) > F2FS file system (version 1.14) > > $ disktype /dev/sda2 > --- /dev/sda2 > Block device, size 17.48 GiB (18772656128 bytes) > NTFS file system > Volume size 17.48 GiB (18772652032 bytes, 36665336 sectors) > > > > > > > > > - End forwarded message - > -- > Steve
Re: [PATCH v8 1/1] plainmount: Support plain encryption mode
--- Original Message ---
On Tuesday, November 29th, 2022 at 20:13, Daniel Kiper
wrote:
> Hmmm... Why do you say "Writing data" here if the GRUB does not support
> filesystems writes except grubenv file? I think it should be clarified
> what you mean or dropped if it is wrong in this context. Otherwise it is
> confusing.
I do not know 100% of GRUB to be sure that none of modules can write data
(can be used to write data) to unformatted disk partitions. Note, the risk
here is not to write to some proper fs, but to write data at arbitrary disk
or partition location which happens to be incorrectly decrypted fs containing
some data (wrongly decrypted plain mode device looks like random data). If
there is nothing in GRUB which can write data at arbitrary disk locations,
then the sentence can be indeed removed. Although writing data at arbitrary
locations is a strange thing to do, doing so on unformatted disk/partition is
safe, while doing so on incorrectly decrypted in plain mode device will damage
encrypted data. If using plain mode considered expert level, then the warning
may be not needed because it is assumed that a user knows what he is doing.
> > +/ Configure cryptodisk uuid */
> > +static void plainmount_set_uuid (grub_cryptodisk_t dev, const char
> > user_uuid)
> > +{
> > + grub_size_t pos = 0;
> > +
> > + / Size of user_uuid is checked in main func */
> > + if (user_uuid != NULL)
> > + grub_memcpy (dev->uuid, user_uuid, grub_strlen (user_uuid));
>
>
> I think grub_strcpy() instead of grub_memcpy() would be more natural in
> string contexts. And then you do not need grub_strlen().
OK.
> Is it safe to assume here that dev->uuid has been zeroed earlier?
It seems Glenn has already answered here.
> > + / Check uuid length */
> > + if (state[OPTION_UUID].set && grub_strlen (state[OPTION_UUID].arg) >
> > + sizeof (PLAINMOUNT_DEFAULT_UUID))
> > + return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > + N_("specified UUID exceeds maximum size"));
>
>
> What will happen if somebody passes shorter, probably non-valid, UUID?
> I think we should check UUID is not shorter than something too.
It seems there is agreement to check that UUID size is longer than 1 and less
than PLAINMOUNT_UUID - 1.
Best regards,
Maxim Fomin
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v8 1/1] plainmount: Support plain encryption mode
--- Original Message ---
On Friday, December 2nd, 2022 at 0:00, Glenn Washburn
wrote:
>
>
> On Sat, 29 Oct 2022 17:40:42 +
> Maxim Fomin ma...@fomin.one wrote:
>
> > From 2b1d2deb3f2416cbc3e7d25cbc4141a3078eaf68 Mon Sep 17 00:00:00 2001
> > From: Maxim Fomin ma...@fomin.one
> > Date: Sat, 29 Oct 2022 18:18:58 +0100
> > Subject: [PATCH v8 1/1] 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 ma...@fomin.one
> >
> > Difference with v7:
>
>
> Daniel pointed this out, but this isn't a well formed patch. I do very
> much appreciate you adding the differences in as it made it easier to
> look at this. And my suggestion was to use --interdiff or --range-diff
> to the format-patch command, which would properly format things. It
> looks like you just copy pasted the output of "git diff v7 v8".
I will try to fix these issues in v9.
> I'm now compiling this patch and found a few compile issues below.
> You're compile testing this right?
First versions of the patch were tested in pure grub src directory.
Later I switched to directly making and installing GRUB package for
my distro using its source script syntax. It seems this process was
affected by environment options which hided these errors/warnings.
I test the patch on my two old laptops - one with UEFI BIOS (x86_64-efi)
and one is pre-UEFI (i386-pc). I was compiling i386-pc target too,
because otherwise the second laptop was unbootable. During i386-pc
compilation I noticed some warnings related to 'PRIuGRUB_XXX'
macros which were absent during efi target compilation. I noticed
that there are similar warnings in other modules and decided that
there are issues with 'PRIuGRUB_XXX' macros at i386-pc platform at
global level. In any case, these issues didn't cause compilation
fail in my working environment because I would not be able to compile
and boot pre-UEFI lap. Do you use -Werror?
P.S. Also thanks for suggested fixes.
Best regards,
Maxim Fomin
> > diff --git a/docs/grub.texi b/docs/grub.texi
> > index 377969984..34ca6b4f1 100644
> > --- a/docs/grub.texi
> > +++ b/docs/grub.texi
> > @@ -5138,13 +5138,13 @@ to generate password hashes. @xref{Security}.
> >
> > Setup access to the encrypted device in plain mode. Offset of the
> > encrypted -data at the device is specified in terms of 512 byte
> > sectors with the blocklist +data at the device is specified in terms
> > of 512 byte sectors using the blocklist syntax and loopback device.
> > The following example shows how to specify 1MiB offset:
> >
> > @example
> > loopback node (hd0,gpt1)2048+
> > -plainmount node
> > +plainmount node @var{...}
> > @end example
> >
> > The @command{plainmount} command can be used to open LUKS encrypted
> > volume @@ -5155,13 +5155,14 @@ The keyfile path parameter has higher
> > priority than the secret passphrase parameter and is specified with
> > the option @option{-d}. Password data obtained from keyfiles is not
> > hashed and is used directly as a cipher key. An optional offset of
> > password data in the keyfile can be specified with the option
> > -@option{-O} or directly with the option @option{-d} and GRUB
> > blocklist syntax. +@option{-O} or directly with the option
> > @option{-d} and GRUB blocklist syntax, +if the keyfile data can be
> > accessed from a device and is 512 byte aligned. The following example
> > shows both methods to specify password data in the keyfile at offset
> > 1MiB:
> >
> > @example
> > -plainmount -d (hd0,gpt1)2048+
> > -plainmount -d (hd0,gpt1)+ -O 1048576
> > +plainmount -d (hd0,gpt1)2048+ @var{...}
> > +plainmount -d (hd0,gpt1)+ -O 1048576 @var{...}
> > @end example
> >
> > If no keyfile is specified then the password is set to the string
> > specified diff --git a/grub-core/disk/plainmount.c
> > b/grub-core/disk/plainmount.c index 656c5d09f..85ada25bc 100644
> > --- a/grub-core/disk/plainmount.c
> > +++ b/grub-core/disk/plainmount.c
> > @@ -146,8 +146,12 @@ plainmount_configure_password (grub_cryptodisk_t
> > dev, const char *hash, dev->hash = grub_crypto_lookup_md_by_name
> > (hash); len = dev->hash->mdlen;
> >
> > - alloc_size = password_size >= key_size ? password_size : key_size;
> > - p = grub_zalloc (alloc_size + (alloc_size / len));
> > + alloc_size = grub_max (password_size, key_size);
> > + /*
> > + * Allocate buffer for the password and for an added prefix
> > character
> > + * for each hash round ('alloc_size' may not be a multiple of
> > 'len').
> > + */
> > + p = grub_zalloc (alloc_size + (alloc_size / len) + 1);
> > derived_hash = grub_zalloc (GRUB_CRYPTODISK_MAX_KEYLEN * 2);
> > if (p == NULL || derived_hash == NULL)
> > {
> > @@ -170,9 +174,10 @@ plainmount_configure_password (grub_cryptodisk_t
> > dev, const char *hash, if (len > size)
> > len = size;
> >
> > - grub_crypto_hash (dev->hash, dh, p, password_size);
> > + grub_crypto_hash (dev->
