Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
On Mon, Dec 07, 2020 at 10:04:01PM -0600, Glenn Washburn wrote:
> On Sun, 6 Dec 2020 20:35:13 +0100
> Patrick Steinhardt wrote:
>
> > On Fri, Dec 04, 2020 at 10:43:41AM -0600, Glenn Washburn wrote:
> > > First, check to make sure that source disk has a known size. If
> > > not, print debug message and return error. There are 4 cases where
> > > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and
> > > uboot), and in all those cases processing continues. So this is
> > > probably a bit conservative. However, 3 of the cases seem
> > > pathological, and the other, biosdisk, happens when booting from a
> > > cd. Since I doubt booting from a LUKS2 volume on a cd is a big use
> > > case, we'll error until someone complains.
> > >
> > > Do some sanity checking on data coming from the luks header. If
> > > segment.size is "dynamic",
> >
> > Nit: there's something missing here.
>
> Yep, thanks for catching that. I was going to complete this and forgot
> in my rush to get the series out before some travel. I'll rework that.
>
> > > Check for errors from grub_strtoull when converting segment size
> > > from string. If a GRUB_ERR_BAD_NUMBER error was returned, then the
> > > string was not a valid parsable number, so skip the key. If
> > > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in
> > > converting to a 64-bit unsigned integer. So this could be a very
> > > large disk (perhaps large raid array). In this case, we want to
> > > continue to try to use this key so long as the source disk's size
> > > is greater than this segment size. Otherwise, this is an invalid
> > > segment, and continue on to the next key.
> > >
> > > Signed-off-by: Glenn Washburn
> > > ---
> > > grub-core/disk/luks2.c | 98
> > > +- include/grub/disk.h|
> > > 17 2 files changed, 105 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index de2e70796..1bb3a333d 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > > grub_luks2_digest_t *d, grub_luks2_s break;
> > > }
> > >if (i == size)
> > > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key);
> > > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key);
> > >/* Get segment that matches the digest. */
> > >if (grub_json_getvalue (, root, "segments") ||
> > > @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > > grub_luks2_digest_t *d, grub_luks2_s break;
> > > }
> > >if (i == size)
> > > -return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > > digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key);
> > > +return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > > digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key);
> > >return GRUB_ERR_NONE;
> > > }
> > > @@ -600,37 +600,115 @@ luks2_recover_key (grub_disk_t source,
> > >goto err;
> > > }
> > >
> > > + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> > > +{
> > > + /* FIXME: Allow use of source disk, and maybe cause errors
> > > in read. */
> > > + grub_dprintf ("luks2", "Source disk %s has an unknown size, "
> > > + "conservatively returning error\n",
> > > source->name);
> > > + ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2
> > > source device");
> > > + goto err;
> > > +}
> > > +
> > >/* Try all keyslot */
> > >for (i = 0; i < size; i++)
> > > {
> > > + typeof(source->total_sectors) max_crypt_sectors = 0;
> >
> > Please bear with me if this has been discussed in a previous round,
> > but why exactly do we cast `max_crypt_sectors` to the type of
> > `source->total_sectors`?
>
> Technically, this isn't a cast. Its a variable declaration. But I'm
> using the typeof(source->total_sectors) because max_crypt_sectors can
> be no more or less than the total sectors, ie its of the same type.
Oh, of course. I somehow didn't realize this at all, probably because
this is not something one sees that often.
> > And isn't the variable always set anyway in
> > case the keyslot has a non-zero priority?
>
> Yep. Are you suggesting that it need not be initialized? This is true,
> but I don't think that's a problem. I think generally initializing
> things is a good practice. It might be problematic if this was in an
> oft used function (or it might not, would need to see if the compiler
> was smart enough to ignore the initialization). But that
> initialization is going to happen very rarely in the lifetime of a grub
> execution instance. I also don't really care either way.
>
> Glenn
I just didn't realize it's a declaration, so keeping the initialization
is fine.
Patrick
signature.asc
Description: PGP
Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks2_keyslot/segment/digest
On Mon, Dec 07, 2020 at 10:06:44PM -0600, Glenn Washburn wrote: > On Mon, 7 Dec 2020 21:02:39 +0100 > Daniel Kiper wrote: > > > On Sun, Dec 06, 2020 at 02:29:06PM +0100, Patrick Steinhardt wrote: > > > On Fri, Dec 04, 2020 at 10:43:34AM -0600, Glenn Washburn wrote: > > > > This allows code using these structs to know the named key > > > > associated with these json data structures. In the future we can > > > > use these to provide better error messages to the user. > > > > > > > > Get rid of idx variable in luks2_get_keyslot() which was > > > > overloaded to be used for both keyslot and segment slot keys. > > > > > > > > Signed-off-by: Glenn Washburn > > > > > > Personally, I'd have named them `json_slot_idx`. But you've already > > > done so much work on improving the code that I don't want this to > > > be the reason to not give an SOB, especially considering that it's > > > a strict improvement anyway. So: > > > > > > Signed-off-by: Patrick Steinhardt > > > > I can change this to json_slot_idx before committing if Glenn does not > > object. Otherwise Reviewed-by: Daniel Kiper > > Thanks Patrick for the sentiment. Looking at the luks2 spec, it says: > > "JSON objects must have their names formatted as a string that > represents a number in the decimal notation (unsigned integer) – > for example ”0”, ”1” and must contain attribute _type_. According to > the _type_, the implementation decides how to handle (or > ignore) such an object. This notation allows mapping to LUKS1 API > functions that use an integer as a reference to keyslots objects." > > So here, the spec is calling that value a "name", and saying that it > must be a string of decimal digits. Looking at the spec, the "name" of > the keyslot object does not need to correspond to the index into the > array of keyslot areas on disk. However it does in the canonical > implementation for use with LUKS1 API functions which require and > integer, as suggested in the quote above. > > I'd say that these numbers are actually an id for the object of its > respective class. In the cryptsetup implementation, the "id" happens > to correspond to the index into the binary key area array, but that's > needn't be the case. My preference would be to name it "id" and second > choice would be just "idx" (since its usually an index into a physical > array of key areas or virtual array of segments and digests). > > I don't think the "json" part of the name is warranted, because it > really has nothing to do with json. The "slot" part is really only > applicable to keyslots because digests and segments don't have an > equivalent slot aspect. So I suggest we name the struct member names > to just "id" instead. And where we just the index of the name-value > pair in the json associative array we use "json_idx" as a suffix. So > this would mean changing the argument keyslot_idx in > luks2_get_keyslot() to keyslot_json_idx. Optionally the local variable > "i" in luks2_get_keyslot() and luks2_parse_segment() could be renamed > to "json_idx" as well (I don't care either way about this though). > > Glenn Sounds sensible to me. Based on your reasoning, I'm happy with either "id" or "key". So we may just as well just keep it as-is, as you prefer. Patrick signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCHv2] grub-install: Add backup and restore
On Tue, 8 Dec 2020, 03:17 Michael Chang, wrote:
> On Mon, Dec 07, 2020 at 12:37:28PM +, Dimitri John Ledkov wrote:
> > Refactor clean_grub_dir to create a backup of all the files, instead
> > of just irrevocably removing them as the first action. If available,
> > register on_exit handle to restore the backup if any errors occur, or
> > remove the backup if everything was successful. If on_exit is not
> > available, the backup remains on disk for manual recovery.
> >
> > This allows safer upgrades of MBR & modules, such that
> > modules/images/fonts/translations are consistent with MBR in case of
> > errors. For example accidental grub-install /dev/non-existent-disk
> > currently clobbers and upgrades modules in /boot/grub, despite not
> > actually updating any MBR. This increases peak disk-usage slightly, by
> > requiring temporarily twice the disk space to complete grub-install.
>
> I'd love to have the described problem fixed too, as it helps a lot in
> the update path to survive grub-install error which can be contributed
> by many different reasons, even though grub has to stay with old version
> but still much better than unbootable system.
>
> But here I have a concern, as to what will happen if the error comes
> after image installation ? For example, the efibootmgr may fail to
> register boot entry after copying the images to efi partition. It looks
> to me that the restoring backup would be triggerd incorrectly for the
> new image to load restored old backups.
>
> Would you please help to clarify that ?
>
On Ubuntu we install secureboot signed prebuilt EFI images only which
prohibit module loading. Hence usually it is irrelevant if the EFI grub
modules are old or new.
And we do not call efibootmgr at all, as it does excessive writes to
efivars. Instead we don't even try to update efivars if there is no need.
It was submitted to this mailing list but I am not sure on the acceptance
status https://lists.gnu.org/archive/html/grub-devel/2019-03/msg00123.html
Above two things make EFI updates less fatal, as it is harder for a
prebuilt signed grub, which does not use modules, to fail booting. Unlike
i386-pc case.
And to answer your immediate question - the new EFI image will be used for
boot without rolling back.
Also, I am not sure how everyone installs signed grubs. Thus adding support
for ESP backup and restore might be futile upstream. As far as I can tell
it is safe to call grub-install on Ubuntu, whereas it might not be on other
distros. As Ubuntu preserves / reinstalls signed grub EFI images, instead
of generating a new random one and putting it on ESP thus causing failure
to boot with secureboot due to effectively replacing signed grub with an
unsigned one. And for resilient boot we have support for maintaining and
synchronising multiple ESP for Linux raid.
I kind of wish we'd prebuilt more core images at package build time and
simply copy them around. Not just the ones that have signing. Instead of
invoking mkimage on every installed system.
Nonetheless, it should be possible to hook up more files and directories to
perform backup, cleanup and restore on. Is there is a desire. The function
calls simply need to be added in the appropriate places.
> >
> > Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
> > it is also cleaned / backed up / restored.
> >
> > Signed-off-by: Dimitri John Ledkov
> > ---
> >
> > Changes since v1: as reported on linux-ext4 mailing list and debugged
> > by Colin Watson, the grub_util_path_concat_ext call was incorrect in
> > the restore backup case as there was no extention needed. Instead the
> > call is corrected to just grub_util_path_concat.
> >
> > This patch is now shipped in Ubuntu & Debian in multiple series. It
> > would be nice to have this merged upstream, as it greatly improves
> > grub upgrades and prevents missmatches of MBR & modules.
> >
> > configure.ac | 2 +-
> > util/grub-install-common.c | 105 +++--
> > 2 files changed, 91 insertions(+), 16 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 7c10a4db7..71cd414c3 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -419,7 +419,7 @@ else
> > fi
> >
> > # Check for functions and headers.
> > -AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
> > +AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
> > AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
> >
> > # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits
> deprecation
> > diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> > index 277eaf4e2..74584b92b 100644
> > --- a/util/grub-install-common.c
> > +++ b/util/grub-install-common.c
> > @@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
> >free (t);
> > }
> >
> > +static int
> > +strcmp_ext (const char *a, const char *b, const char *ext)
> > +{
> > + char *bsuffix = grub_util_path_concat_ext (1,
Re: [PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead
On Fri, 4 Dec 2020 13:34:44 +0100 Daniel Kiper wrote: > On Thu, Dec 03, 2020 at 07:57:11PM -0600, Glenn Washburn wrote: > > If there is a loopback device with the same name as the one to be > > created, instead of closing the old one and replacing it with the > > new one, return an error instead. If the loopback device was > > created, its probably being used by something and just replacing it > > may cause grub to crash unexpectedly. This fixes obvious problems > > like `loopback d (d)/somefile'. Its not too onerous to force the > > user to delete the loopback first with the `-d' switch. > > > > Signed-off-by: Glenn Washburn > > Reviewed-by: Daniel Kiper > > Daniel > > PS May I ask you to create new thread for new version of the patches >instead of attaching them to previous threads? These two patches were not meant to be a thread together, ie separate patches, which was why I was treating them separately. I can see how that might be confusing now. Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: Mips-arc tests ever work?
On Sat, 5 Dec 2020 21:28:05 +0100 John Paul Adrian Glaubitz wrote: > On 12/5/20 8:10 PM, Glenn Washburn wrote: > >> I'm running manual tests for all architectures before each release. > > > > What exactly are manual tests? Is that running manually the tests > > for that target? (ie "make check" for that target) > > Yes. I'm test-compiling GRUB natively, run "make check" and also > perform test installations in some cases. Ok so I'm clear, since you're running "make check", your qemu tests for mips-arc are failing due to indy not being a machine type, correct? > > Also what version of qemu are you running to do the tests? > > I'm performing all tests on real hardware. I have access to all > architectures supported by GRUB either through my personal hardware > zoo, the Debian project of which I am member of or the GCC compile > farm for which I am also providing hardware. > > You are free to register for the GCC compile farm and use it yourself > [1]. Cool project, thanks for the tip. > >> I could not find any reference to "indy" in the qemu git log. Are > >> you sure it was ever officially supported? I assume the tests here > >> were run on real hardware. > > > > Take a look in "tests/util/grub-shell.in" and you'll find the > > offending test code in grub. I have no clue either way whether it > > was ever supported, but someone obviously did because its in the > > test code. And that someone might be Vladimir (phcoder) who > > committed the code. This is why I'm wondering if it ever worked. > > Maybe he used a fork of qemu for these tests. > > >>> According to wikipedia the SGI Indy originally used R4k > >>> processors. So would machine type mips be the one to use since > >>> its description says "mips r4k platform"? > >> > >> No, that was a different, generic machine type, see [1]. > > > > I'm confused by this response. My qemu doesn't list a "r4k" machine > > type and its version 4.2.1. The machine type "mips" says its the > > r4k platform, but that doesn't look like what's being removed in > > that commit. Reviewing the hw/mips/Kconfig, it looks like the > > machine types have changed a lot since 4.2.1, so maybe it was > > renamed to "r4k" (haven't checked yet). However if it was renamed > > and is being removed, that's going to cause further problems > > (when/if we use qemu 5.2 for testing) because grub-shell is using > > the "mips" machine type for "mips-qemu_mips" and "mipsel-qemu_mips". > > Well, either way, the r4k machine no longer exists in QEMU 5.2. > > >>> So can anyone familiar with this tell me an appropriate machine > >>> type of the above listed to use instead of indy? Perhaps Vladimir > >>> who committed that code can chime in? > > > > Ultimately, I just want to know what machine should I use if not > > indy for the "mips-arc" target? I know very little about mips and > > what an appropriate machine type would be for emulating this target. > > I guess "malta" would work, too. Ok, see if that works. > Adrian > > > [1] https://gcc.gnu.org/wiki/CompileFarm > ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
This patch got mangled accidentally with patch 5. I'll update them
both.
Glenn
On Fri, 4 Dec 2020 10:43:41 -0600
Glenn Washburn wrote:
> First, check to make sure that source disk has a known size. If not,
> print debug message and return error. There are 4 cases where
> GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and uboot),
> and in all those cases processing continues. So this is probably a bit
> conservative. However, 3 of the cases seem pathological, and the
> other, biosdisk, happens when booting from a cd. Since I doubt
> booting from a LUKS2 volume on a cd is a big use case, we'll error
> until someone complains.
>
> Do some sanity checking on data coming from the luks header. If
> segment.size is "dynamic",
>
> Check for errors from grub_strtoull when converting segment size from
> string. If a GRUB_ERR_BAD_NUMBER error was returned, then the string
> was not a valid parsable number, so skip the key. If
> GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in
> converting to a 64-bit unsigned integer. So this could be a very
> large disk (perhaps large raid array). In this case, we want to
> continue to try to use this key so long as the source disk's size is
> greater than this segment size. Otherwise, this is an invalid
> segment, and continue on to the next key.
>
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/luks2.c | 98
> +- include/grub/disk.h|
> 17 2 files changed, 105 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index de2e70796..1bb3a333d 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s break;
> }
>if (i == size)
> - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key);
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key);
>/* Get segment that matches the digest. */
>if (grub_json_getvalue (, root, "segments") ||
> @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s break;
> }
>if (i == size)
> -return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key);
> +return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key);
>return GRUB_ERR_NONE;
> }
> @@ -600,37 +600,115 @@ luks2_recover_key (grub_disk_t source,
>goto err;
> }
>
> + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> +{
> + /* FIXME: Allow use of source disk, and maybe cause errors in
> read. */
> + grub_dprintf ("luks2", "Source disk %s has an unknown size, "
> + "conservatively returning error\n",
> source->name);
> + ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2 source
> device");
> + goto err;
> +}
> +
>/* Try all keyslot */
>for (i = 0; i < size; i++)
> {
> + typeof(source->total_sectors) max_crypt_sectors = 0;
> +
> + grub_errno = GRUB_ERR_NONE;
>ret = luks2_get_keyslot (, , , json, i);
>if (ret)
> goto err;
> + if (grub_errno != GRUB_ERR_NONE)
> + grub_dprintf ("luks2", "Ignoring unhandled error %d from
> luks2_get_keyslot\n", grub_errno);
>if (keyslot.priority == 0)
> {
> - grub_dprintf ("luks2", "Ignoring keyslot
> \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.slot_key);
> + grub_dprintf ("luks2", "Ignoring keyslot
> \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.json_slot_key);
> continue; }
>
> - grub_dprintf ("luks2", "Trying keyslot
> \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.slot_key);
> + grub_dprintf ("luks2", "Trying keyslot
> \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.json_slot_key);
>/* Set up disk according to keyslot's segment. */
>crypt->offset_sectors = grub_divmod64 (segment.offset,
> segment.sector_size, NULL); crypt->log_sector_size = sizeof (unsigned
> int) * 8
> - __builtin_clz ((unsigned int) segment.sector_size)
> - 1;
> + /* Set to the source disk size, which is the maximum we allow.
> */
> + max_crypt_sectors = grub_disk_convert_sector(source,
> +
> source->total_sectors,
> +
> crypt->log_sector_size); +
> + if (max_crypt_sectors < crypt->offset_sectors)
> + {
> + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> has offset"
> + " %"PRIuGRUB_UINT64_T" which is
> greater than"
> + " source disk size
> %"PRIuGRUB_UINT64_T","
> + " skipping\n",
> + segment.json_slot_key,
> crypt->offset_sectors,
> + max_crypt_sectors);
> +
Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks2_keyslot/segment/digest
On Mon, 7 Dec 2020 21:02:39 +0100 Daniel Kiper wrote: > On Sun, Dec 06, 2020 at 02:29:06PM +0100, Patrick Steinhardt wrote: > > On Fri, Dec 04, 2020 at 10:43:34AM -0600, Glenn Washburn wrote: > > > This allows code using these structs to know the named key > > > associated with these json data structures. In the future we can > > > use these to provide better error messages to the user. > > > > > > Get rid of idx variable in luks2_get_keyslot() which was > > > overloaded to be used for both keyslot and segment slot keys. > > > > > > Signed-off-by: Glenn Washburn > > > > Personally, I'd have named them `json_slot_idx`. But you've already > > done so much work on improving the code that I don't want this to > > be the reason to not give an SOB, especially considering that it's > > a strict improvement anyway. So: > > > > Signed-off-by: Patrick Steinhardt > > I can change this to json_slot_idx before committing if Glenn does not > object. Otherwise Reviewed-by: Daniel Kiper Thanks Patrick for the sentiment. Looking at the luks2 spec, it says: "JSON objects must have their names formatted as a string that represents a number in the decimal notation (unsigned integer) – for example ”0”, ”1” and must contain attribute _type_. According to the _type_, the implementation decides how to handle (or ignore) such an object. This notation allows mapping to LUKS1 API functions that use an integer as a reference to keyslots objects." So here, the spec is calling that value a "name", and saying that it must be a string of decimal digits. Looking at the spec, the "name" of the keyslot object does not need to correspond to the index into the array of keyslot areas on disk. However it does in the canonical implementation for use with LUKS1 API functions which require and integer, as suggested in the quote above. I'd say that these numbers are actually an id for the object of its respective class. In the cryptsetup implementation, the "id" happens to correspond to the index into the binary key area array, but that's needn't be the case. My preference would be to name it "id" and second choice would be just "idx" (since its usually an index into a physical array of key areas or virtual array of segments and digests). I don't think the "json" part of the name is warranted, because it really has nothing to do with json. The "slot" part is really only applicable to keyslots because digests and segments don't have an equivalent slot aspect. So I suggest we name the struct member names to just "id" instead. And where we just the index of the name-value pair in the json associative array we use "json_idx" as a suffix. So this would mean changing the argument keyslot_idx in luks2_get_keyslot() to keyslot_json_idx. Optionally the local variable "i" in luks2_get_keyslot() and luks2_parse_segment() could be renamed to "json_idx" as well (I don't care either way about this though). Glenn ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals
On Mon, 7 Dec 2020 21:22:09 +0100
Daniel Kiper wrote:
> On Fri, Dec 04, 2020 at 10:43:38AM -0600, Glenn Washburn wrote:
> > Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for
> > an unsigned number with size of type.
> >
> > Signed-off-by: Glenn Washburn
>
> Reviewed-by: Daniel Kiper
>
> But one nit below...
>
> > ---
> > grub-core/disk/cryptodisk.c | 8
> > include/grub/types.h| 7 +++
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 0e955a020..5aa0c4720 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, iv[1] = grub_cpu_to_le32 (sector >>
> > GRUB_TYPE_BITS (iv[0])); /* FALLTHROUGH */
> > case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> > - iv[0] = grub_cpu_to_le32 (sector & 0x);
> > + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); break;
> > case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> > iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS
> > (iv[1])
> >-
> > dev->log_sector_size)); iv[0] = grub_cpu_to_le32 ((sector <<
> > dev->log_sector_size)
> > - & 0x);
> > + & GRUB_TYPE_U_MAX (iv[0]));
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_BENBI:
> > {
> > grub_uint64_t num = (sector << dev->benbi_log) + 1;
> > iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BITS
> > (iv[0]));
> > - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> > + iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX
> > (iv[0])); }
> > break;
> > case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> > - iv[0] = grub_cpu_to_le32 (sector & 0x);
> > + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX
> > (iv[0])); err = grub_crypto_ecb_encrypt (dev->essiv_cipher, iv, iv,
> > dev->cipher->cipher->blocksize);
> > if (err)
> > diff --git a/include/grub/types.h b/include/grub/types.h
> > index 9989e3a16..0542011cc 100644
> > --- a/include/grub/types.h
> > +++ b/include/grub/types.h
> > @@ -161,6 +161,13 @@ typedef grub_int32_t grub_ssize_t;
> > #endif
> > # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
> >
> > +/*
> > + Cast to unsigned long long so that the "return value" is always
> > a consistent
> > + type and to ensure an unsigned value regardless of type
> > parameter.
> > + */
>
> Incorrect comment formatting...
I'll fix it, and get it right one of these days.
> > +#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof
> > (type))(~0))) +#define GRUB_TYPE_U_MIN(type) 0ULL
> > +
> > typedef grub_uint64_t grub_properly_aligned_t;
> >
> > #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size)
> > grub_properly_aligned_t name[((size) + sizeof
> > (grub_properly_aligned_t) - 1) / sizeof (grub_properly_aligned_t)]
>
> Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
On Sun, 6 Dec 2020 20:35:13 +0100
Patrick Steinhardt wrote:
> On Fri, Dec 04, 2020 at 10:43:41AM -0600, Glenn Washburn wrote:
> > First, check to make sure that source disk has a known size. If
> > not, print debug message and return error. There are 4 cases where
> > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and
> > uboot), and in all those cases processing continues. So this is
> > probably a bit conservative. However, 3 of the cases seem
> > pathological, and the other, biosdisk, happens when booting from a
> > cd. Since I doubt booting from a LUKS2 volume on a cd is a big use
> > case, we'll error until someone complains.
> >
> > Do some sanity checking on data coming from the luks header. If
> > segment.size is "dynamic",
>
> Nit: there's something missing here.
Yep, thanks for catching that. I was going to complete this and forgot
in my rush to get the series out before some travel. I'll rework that.
> > Check for errors from grub_strtoull when converting segment size
> > from string. If a GRUB_ERR_BAD_NUMBER error was returned, then the
> > string was not a valid parsable number, so skip the key. If
> > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in
> > converting to a 64-bit unsigned integer. So this could be a very
> > large disk (perhaps large raid array). In this case, we want to
> > continue to try to use this key so long as the source disk's size
> > is greater than this segment size. Otherwise, this is an invalid
> > segment, and continue on to the next key.
> >
> > Signed-off-by: Glenn Washburn
> > ---
> > grub-core/disk/luks2.c | 98
> > +- include/grub/disk.h|
> > 17 2 files changed, 105 insertions(+), 10 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index de2e70796..1bb3a333d 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > grub_luks2_digest_t *d, grub_luks2_s break;
> > }
> >if (i == size)
> > - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key);
> > + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key);
> >/* Get segment that matches the digest. */
> >if (grub_json_getvalue (, root, "segments") ||
> > @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > grub_luks2_digest_t *d, grub_luks2_s break;
> > }
> >if (i == size)
> > -return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key);
> > +return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key);
> >return GRUB_ERR_NONE;
> > }
> > @@ -600,37 +600,115 @@ luks2_recover_key (grub_disk_t source,
> >goto err;
> > }
> >
> > + if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> > +{
> > + /* FIXME: Allow use of source disk, and maybe cause errors
> > in read. */
> > + grub_dprintf ("luks2", "Source disk %s has an unknown size, "
> > +"conservatively returning error\n",
> > source->name);
> > + ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2
> > source device");
> > + goto err;
> > +}
> > +
> >/* Try all keyslot */
> >for (i = 0; i < size; i++)
> > {
> > + typeof(source->total_sectors) max_crypt_sectors = 0;
>
> Please bear with me if this has been discussed in a previous round,
> but why exactly do we cast `max_crypt_sectors` to the type of
> `source->total_sectors`?
Technically, this isn't a cast. Its a variable declaration. But I'm
using the typeof(source->total_sectors) because max_crypt_sectors can
be no more or less than the total sectors, ie its of the same type.
> And isn't the variable always set anyway in
> case the keyslot has a non-zero priority?
Yep. Are you suggesting that it need not be initialized? This is true,
but I don't think that's a problem. I think generally initializing
things is a good practice. It might be problematic if this was in an
oft used function (or it might not, would need to see if the compiler
was smart enough to ignore the initialization). But that
initialization is going to happen very rarely in the lifetime of a grub
execution instance. I also don't really care either way.
Glenn
> Patrick
>
> > +
> > + grub_errno = GRUB_ERR_NONE;
> >ret = luks2_get_keyslot (, , , json,
> > i); if (ret)
> > goto err;
> > + if (grub_errno != GRUB_ERR_NONE)
> > + grub_dprintf ("luks2", "Ignoring unhandled error %d from
> > luks2_get_keyslot\n", grub_errno);
> >if (keyslot.priority == 0)
> > {
> > - grub_dprintf ("luks2", "Ignoring keyslot
> > \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.slot_key);
> > + grub_dprintf ("luks2", "Ignoring
Re: [PATCHv2] grub-install: Add backup and restore
On Mon, Dec 07, 2020 at 12:37:28PM +, Dimitri John Ledkov wrote:
> Refactor clean_grub_dir to create a backup of all the files, instead
> of just irrevocably removing them as the first action. If available,
> register on_exit handle to restore the backup if any errors occur, or
> remove the backup if everything was successful. If on_exit is not
> available, the backup remains on disk for manual recovery.
>
> This allows safer upgrades of MBR & modules, such that
> modules/images/fonts/translations are consistent with MBR in case of
> errors. For example accidental grub-install /dev/non-existent-disk
> currently clobbers and upgrades modules in /boot/grub, despite not
> actually updating any MBR. This increases peak disk-usage slightly, by
> requiring temporarily twice the disk space to complete grub-install.
I'd love to have the described problem fixed too, as it helps a lot in
the update path to survive grub-install error which can be contributed
by many different reasons, even though grub has to stay with old version
but still much better than unbootable system.
But here I have a concern, as to what will happen if the error comes
after image installation ? For example, the efibootmgr may fail to
register boot entry after copying the images to efi partition. It looks
to me that the restoring backup would be triggerd incorrectly for the
new image to load restored old backups.
Would you please help to clarify that ?
Thanks,
Michael
>
> Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
> it is also cleaned / backed up / restored.
>
> Signed-off-by: Dimitri John Ledkov
> ---
>
> Changes since v1: as reported on linux-ext4 mailing list and debugged
> by Colin Watson, the grub_util_path_concat_ext call was incorrect in
> the restore backup case as there was no extention needed. Instead the
> call is corrected to just grub_util_path_concat.
>
> This patch is now shipped in Ubuntu & Debian in multiple series. It
> would be nice to have this merged upstream, as it greatly improves
> grub upgrades and prevents missmatches of MBR & modules.
>
> configure.ac | 2 +-
> util/grub-install-common.c | 105 +++--
> 2 files changed, 91 insertions(+), 16 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 7c10a4db7..71cd414c3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -419,7 +419,7 @@ else
> fi
>
> # Check for functions and headers.
> -AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
> +AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
> AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
>
> # glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits
> deprecation
> diff --git a/util/grub-install-common.c b/util/grub-install-common.c
> index 277eaf4e2..74584b92b 100644
> --- a/util/grub-install-common.c
> +++ b/util/grub-install-common.c
> @@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
>free (t);
> }
>
> +static int
> +strcmp_ext (const char *a, const char *b, const char *ext)
> +{
> + char *bsuffix = grub_util_path_concat_ext (1, b, ext);
> + int r = strcmp (a, bsuffix);
> + free (bsuffix);
> + return r;
> +}
> +
> +enum clean_grub_dir_mode
> +{
> + CLEAN = 0,
> + CLEAN_BACKUP = 1,
> + CREATE_BACKUP = 2,
> + RESTORE_BACKUP = 3,
> +};
> +
> static void
> -clean_grub_dir (const char *di)
> +clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode)
> {
>grub_util_fd_dir_t d;
>grub_util_fd_dirent_t de;
> + char suffix[2] = "";
> +
> + if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
> +{
> + strcpy (suffix, "-");
> +}
>
>d = grub_util_fd_opendir (di);
>if (!d)
> -grub_util_error (_("cannot open directory `%s': %s"),
> - di, grub_util_fd_strerror ());
> +{
> + if (mode == CLEAN_BACKUP)
> + return;
> + grub_util_error (_("cannot open directory `%s': %s"),
> +di, grub_util_fd_strerror ());
> +}
>
>while ((de = grub_util_fd_readdir (d)))
> {
>const char *ext = strrchr (de->d_name, '.');
> - if ((ext && (strcmp (ext, ".mod") == 0
> -|| strcmp (ext, ".lst") == 0
> -|| strcmp (ext, ".img") == 0
> -|| strcmp (ext, ".mo") == 0)
> -&& strcmp (de->d_name, "menu.lst") != 0)
> - || strcmp (de->d_name, "efiemu32.o") == 0
> - || strcmp (de->d_name, "efiemu64.o") == 0)
> + if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0
> +|| strcmp_ext (ext, ".lst", suffix) == 0
> +|| strcmp_ext (ext, ".img", suffix) == 0
> +|| strcmp_ext (ext, ".mo", suffix) == 0)
> +&& strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
> + || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
> + || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
> + || strcmp_ext (de->d_name, "efiemu64.o",
Re: [PATCH 8/9] efi: Only register shim_lock verifier if shim_lock protocol is found and SB enabled
On Thu, Dec 03, 2020 at 04:01:49PM +0100, Javier Martinez Canillas wrote:
> The shim_lock module registers a verifier to call shim's verify, but the
> handler is registered even when the shim_lock protocol was not installed.
>
> This doesn't cause a NULL pointer dereference in shim_lock_write() because
> the shim_lock_init() function just returns GRUB_ERR_NONE if sl isn't set.
>
> But in that case there's no point to even register the shim_lock verifier
> since won't do anything. Additionally, it is only useful when Secure Boot
> is enabled.
>
> Finally, don't assume that the shim_lock protocol will always be present
> when the shim_lock_write() function is called, and check for it on every
> call to this function.
>
> Reported-by: Michael Chang
To complete the information here, this fixed the problem I tried to
solve before, but in a more elegant way. :)
https://www.mail-archive.com/grub-devel@gnu.org/msg30738.html
Thank you to work on the patch.
Regards,
Michael
> Reported-by: Peter Jones
> Signed-off-by: Javier Martinez Canillas
> ---
>
> grub-core/commands/efi/shim_lock.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/commands/efi/shim_lock.c
> b/grub-core/commands/efi/shim_lock.c
> index d8f52d721c3..5259b27e8fc 100644
> --- a/grub-core/commands/efi/shim_lock.c
> +++ b/grub-core/commands/efi/shim_lock.c
> @@ -28,7 +28,6 @@
> GRUB_MOD_LICENSE ("GPLv3+");
>
> static grub_efi_guid_t shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
> -static grub_efi_shim_lock_protocol_t *sl;
>
> /* List of modules which cannot be loaded if UEFI secure boot mode is
> enabled. */
> static const char * const disabled_mods[] = {"iorw", "memrw", "wrmsr", NULL};
> @@ -43,9 +42,6 @@ shim_lock_init (grub_file_t io, enum grub_file_type type,
>
>*flags = GRUB_VERIFY_FLAGS_SKIP_VERIFICATION;
>
> - if (!sl)
> -return GRUB_ERR_NONE;
> -
>switch (type & GRUB_FILE_TYPE_MASK)
> {
> case GRUB_FILE_TYPE_GRUB_MODULE:
> @@ -100,6 +96,11 @@ shim_lock_init (grub_file_t io, enum grub_file_type type,
> static grub_err_t
> shim_lock_write (void *context __attribute__ ((unused)), void *buf,
> grub_size_t size)
> {
> + grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol
> (_lock_guid, 0);
> +
> + if (sl == NULL)
> +return grub_error (GRUB_ERR_ACCESS_DENIED, N_("shim_lock protocol not
> found"));
> +
>if (sl->verify (buf, size) != GRUB_EFI_SUCCESS)
> return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad shim signature"));
>
> @@ -115,12 +116,13 @@ struct grub_file_verifier shim_lock =
>
> GRUB_MOD_INIT(shim_lock)
> {
> - sl = grub_efi_locate_protocol (_lock_guid, 0);
> - grub_verifier_register (_lock);
> + grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol
> (_lock_guid, 0);
>
> - if (!sl)
> + if (sl == NULL || grub_efi_get_secureboot () !=
> GRUB_EFI_SECUREBOOT_MODE_ENABLED)
> return;
>
> + grub_verifier_register (_lock);
> +
>grub_dl_set_persistent (mod);
> }
>
> --
> 2.28.0
>
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH] templates: allow loading a Device Tree file from user conf
From: Matteo Croce
Some machines rely on Device Tree for hardware discovery, so a
.dtb file must be passed to the kernel.
GRUB can do this via the `devicetree` command, but it's not possible
to do this from the user configuration.
Add a GRUB_DEVICETREE_FILE variable which holds the path of the .dtb
file relative to the boot partition and, if present, adds the
devicetree command to the menuentry.
Signed-off-by: Matteo Croce
---
util/grub-mkconfig.in | 1 +
util/grub.d/10_linux.in | 5 +
2 files changed, 6 insertions(+)
diff --git a/util/grub-mkconfig.in b/util/grub-mkconfig.in
index d3e879b8e..3e15e6f20 100644
--- a/util/grub-mkconfig.in
+++ b/util/grub-mkconfig.in
@@ -228,6 +228,7 @@ export GRUB_DEFAULT \
GRUB_CMDLINE_NETBSD \
GRUB_CMDLINE_NETBSD_DEFAULT \
GRUB_CMDLINE_GNUMACH \
+ GRUB_DEVICETREE_FILE \
GRUB_EARLY_INITRD_LINUX_CUSTOM \
GRUB_EARLY_INITRD_LINUX_STOCK \
GRUB_TERMINAL_INPUT \
diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
index e8b01c0d0..fb1bd30f5 100644
--- a/util/grub.d/10_linux.in
+++ b/util/grub.d/10_linux.in
@@ -143,6 +143,11 @@ linux_entry ()
echo'$(echo "$message" | grub_quote)'
linux ${rel_dirname}/${basename}
root=${linux_root_device_thisversion} ro ${args}
EOF
+ if [ -n "${GRUB_DEVICETREE_FILE}" ] && test -e
"${dirname}/${GRUB_DEVICETREE_FILE}"; then
+sed "s/^/$submenu_indentation/" << EOF
+ devicetree ${rel_dirname}/${GRUB_DEVICETREE_FILE}
+EOF
+ fi
if test -n "${initrd}" ; then
# TRANSLATORS: ramdisk isn't identifier. Should be translated.
message="$(gettext_printf "Loading initial ramdisk ...")"
--
2.28.0
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [SPECIFICATION RFC] The firmware and bootloader log specification
On Fri, Dec 04, 2020 at 02:23:23PM +0100, Paul Menzel wrote: > Dear Wim, dear Daniel, > > > First, thank you for including all parties in the discussion. > Am 04.12.20 um 13:52 schrieb Wim Vervoorn: > > > I agree with you. Using an existing standard is better than inventing > > a new one in this case. I think using the coreboot logging is a good > > idea as there is indeed a lot of support already available and it is > > lightweight and simple. > In my opinion coreboot’s format is lacking, that it does not record the > timestamp, and the log level is not stored as metadata, but (in coreboot) > only used to decide if to print the message or not. > > I agree with you, that an existing standard should be used, and in my > opinion it’s Linux message format. That is most widely supported, and > existing tools could then also work with pre-Linux messages. > > Sean Hudson from Mentor Graphics presented that idea at Embedded Linux > Conference Europe 2016 [1]. No idea, if anything came out of that effort. > (Unfortunately, I couldn’t find an email. Does somebody have contacts at > Mentor to find out, how to reach him?) I believe the main thing that came out of this was the reminder that there was an even older attempt by U-Boot to have such a mechanism, and that at the time getting the work accepted in Linux faced some hurdles or another. That said, I too agree with taking what's already a de facto standard, the coreboot logging, and expand on it as needed. -- Tom signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 09/17] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals
On Fri, Dec 04, 2020 at 10:43:38AM -0600, Glenn Washburn wrote:
> Add GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> unsigned number with size of type.
>
> Signed-off-by: Glenn Washburn
Reviewed-by: Daniel Kiper
But one nit below...
> ---
> grub-core/disk/cryptodisk.c | 8
> include/grub/types.h| 7 +++
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 0e955a020..5aa0c4720 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -284,23 +284,23 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
> iv[1] = grub_cpu_to_le32 (sector >> GRUB_TYPE_BITS (iv[0]));
> /* FALLTHROUGH */
> case GRUB_CRYPTODISK_MODE_IV_PLAIN:
> - iv[0] = grub_cpu_to_le32 (sector & 0x);
> + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> break;
> case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
> iv[1] = grub_cpu_to_le32 (sector >> (GRUB_TYPE_BITS (iv[1])
> - dev->log_sector_size));
> iv[0] = grub_cpu_to_le32 ((sector << dev->log_sector_size)
> - & 0x);
> + & GRUB_TYPE_U_MAX (iv[0]));
> break;
> case GRUB_CRYPTODISK_MODE_IV_BENBI:
> {
> grub_uint64_t num = (sector << dev->benbi_log) + 1;
> iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BITS (iv[0]));
> - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> + iv[sz - 1] = grub_cpu_to_be32 (num & GRUB_TYPE_U_MAX (iv[0]));
> }
> break;
> case GRUB_CRYPTODISK_MODE_IV_ESSIV:
> - iv[0] = grub_cpu_to_le32 (sector & 0x);
> + iv[0] = grub_cpu_to_le32 (sector & GRUB_TYPE_U_MAX (iv[0]));
> err = grub_crypto_ecb_encrypt (dev->essiv_cipher, iv, iv,
>dev->cipher->cipher->blocksize);
> if (err)
> diff --git a/include/grub/types.h b/include/grub/types.h
> index 9989e3a16..0542011cc 100644
> --- a/include/grub/types.h
> +++ b/include/grub/types.h
> @@ -161,6 +161,13 @@ typedef grub_int32_t grub_ssize_t;
> #endif
> # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
>
> +/*
> + Cast to unsigned long long so that the "return value" is always a
> consistent
> + type and to ensure an unsigned value regardless of type parameter.
> + */
Incorrect comment formatting...
> +#define GRUB_TYPE_U_MAX(type) ((unsigned long long)((typeof (type))(~0)))
> +#define GRUB_TYPE_U_MIN(type) 0ULL
> +
> typedef grub_uint64_t grub_properly_aligned_t;
>
> #define GRUB_PROPERLY_ALIGNED_ARRAY(name, size) grub_properly_aligned_t
> name[((size) + sizeof (grub_properly_aligned_t) - 1) / sizeof
> (grub_properly_aligned_t)]
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 08/17] cryptodisk: Add macro GRUB_TYPE_BITS() to replace some literals
On Fri, Dec 04, 2020 at 10:43:37AM -0600, Glenn Washburn wrote: > The new macro GRUB_TYPE_BITS(type) returns the number of bits allocated for > type. > > 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 v7 07/17] luks2: Add string "index" to user strings using a json index.
On Fri, Dec 04, 2020 at 10:43:36AM -0600, Glenn Washburn wrote: > This allows error messages to be more easily distinguishable between indexes > and slot keys. The former include the string "index" in the error/debug > string, and the later are surrounded in quotes. > > 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 v7 06/17] luks2: Use more intuitive slot key instead of index in user messages
On Fri, Dec 04, 2020 at 10:43:35AM -0600, Glenn Washburn wrote:
> Use the slot key name in the json array rather than the 0 based index in the
> json array for keyslots, segments, and digests. This is less confusing for
> the end user. For example, say you have a LUKS2 device with a key in slot 1
> and slot 4. When using the password for slot 4 to unlock the device, the
> messages using the index of the keyslot will mention keyslot 1 (its a
> zero-based index). Furthermore, with this change the keyslot number will
> align with the number used to reference the keyslot when using the
> --key-slot argument to cryptsetup.
>
> Signed-off-by: Glenn Washburn
> ---
> grub-core/disk/luks2.c | 20 ++--
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 437c1da07..ea1065bcf 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -284,13 +284,13 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s
> grub_json_getuint64 (>json_slot_key, , NULL) ||
>grub_json_getchild (, , 0) ||
>luks2_parse_digest (d, ))
> - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest
> %"PRIuGRUB_SIZE, i);
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index
> %"PRIuGRUB_SIZE, i);
Does not this change belong to next patch?
>if ((d->keyslots & (1 << k->json_slot_key)))
> break;
> }
>if (i == size)
> - return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot
> %"PRIuGRUB_SIZE, keyslot_idx);
> + return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot
> \"%"PRIuGRUB_UINT64_T"\"", k->slot_key);
I am afraid this does not build. The slot_key seems a remnant from v6.
>/* Get segment that matches the digest. */
>if (grub_json_getvalue (, root, "segments") ||
> @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> grub_luks2_digest_t *d, grub_luks2_s
> break;
> }
>if (i == size)
> -return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest
> %"PRIuGRUB_SIZE);
> +return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest
> \"%"PRIuGRUB_UINT64_T"\"", d->slot_key);
Ditto and below...
>return GRUB_ERR_NONE;
> }
> @@ -604,11 +604,11 @@ luks2_recover_key (grub_disk_t source,
>
>if (keyslot.priority == 0)
> {
> - grub_dprintf ("luks2", "Ignoring keyslot %"PRIuGRUB_SIZE" due to
> priority\n", i);
> + grub_dprintf ("luks2", "Ignoring keyslot \"%"PRIuGRUB_UINT64_T"\" due
> to priority\n", keyslot.slot_key);
> continue;
> }
>
> - grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
> + grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n",
> keyslot.slot_key);
>
>/* Set up disk according to keyslot's segment. */
>crypt->offset_sectors = grub_divmod64 (segment.offset,
> segment.sector_size, NULL);
> @@ -623,16 +623,16 @@ luks2_recover_key (grub_disk_t source,
> (const grub_uint8_t *) passphrase, grub_strlen
> (passphrase));
>if (ret)
> {
> - grub_dprintf ("luks2", "Decryption with keyslot %"PRIuGRUB_SIZE"
> failed: %s\n",
> - i, grub_errmsg);
> + grub_dprintf ("luks2", "Decryption with keyslot
> \"%"PRIuGRUB_UINT64_T"\" failed: %s\n",
> + keyslot.slot_key, grub_errmsg);
> continue;
> }
>
>ret = luks2_verify_key (, candidate_key, keyslot.key_size);
>if (ret)
> {
> - grub_dprintf ("luks2", "Could not open keyslot %"PRIuGRUB_SIZE":
> %s\n",
> - i, grub_errmsg);
> + grub_dprintf ("luks2", "Could not open keyslot
> \"%"PRIuGRUB_UINT64_T"\": %s\n",
> + keyslot.slot_key, grub_errmsg);
> continue;
> }
>
> @@ -640,7 +640,7 @@ luks2_recover_key (grub_disk_t source,
> * TRANSLATORS: It's a cryptographic key slot: one element of an array
> * where each element is either empty or holds a key.
> */
> - grub_printf_ (N_("Slot %"PRIuGRUB_SIZE" opened\n"), i);
> + grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"),
> keyslot.slot_key);
>
>candidate_key_len = keyslot.key_size;
>break;
Daniel
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks2_keyslot/segment/digest
On Sun, Dec 06, 2020 at 02:29:06PM +0100, Patrick Steinhardt wrote: > On Fri, Dec 04, 2020 at 10:43:34AM -0600, Glenn Washburn wrote: > > This allows code using these structs to know the named key associated with > > these json data structures. In the future we can use these to provide better > > error messages to the user. > > > > Get rid of idx variable in luks2_get_keyslot() which was overloaded to be > > used for both keyslot and segment slot keys. > > > > Signed-off-by: Glenn Washburn > > Personally, I'd have named them `json_slot_idx`. But you've already done > so much work on improving the code that I don't want this to be the > reason to not give an SOB, especially considering that it's a strict > improvement anyway. So: > > Signed-off-by: Patrick Steinhardt I can change this to json_slot_idx before committing if Glenn does not object. Otherwise Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 04/17] luks2: Make sure all fields of output argument in luks2_parse_digest() are written to
On Fri, Dec 04, 2020 at 10:43:33AM -0600, Glenn Washburn wrote: > We should assume that the output argument "out" is uninitialized and could > have random data. So, make sure to initialize the segments and keyslots bit > fields because potentially not all bits of those fields are written to. > Otherwise, the digest could say it belongs to keyslots and segments that it > does not. > > 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 v7 03/17] luks2: Remove unused argument in grub_error
On Fri, Dec 04, 2020 at 10:43:32AM -0600, Glenn Washburn wrote: > Reviewed-by: Daniel Kiper > Signed-off-by: Glenn Washburn Nit, my RB should be after your SOB. I will fix it. Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 02/17] misc: Add parentheses around ALIGN_UP and ALIGN_DOWN arguments
On Fri, Dec 04, 2020 at 10:43:31AM -0600, Glenn Washburn wrote: > This ensures that expected order of operations is preserved when arguments > are expressions. > > Signed-off-by: Glenn Washburn Good catch! Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v7 01/17] disk: Rename grub_disk_get_size to grub_disk_native_sectors
On Fri, Dec 04, 2020 at 10:43:30AM -0600, Glenn Washburn wrote: > The function grub_disk_get_size is confusingly named because it actually > returns a sector count where the sectors are sized in the grub native sector > size. Rename to something more appropriate. > > Suggested-by: Daniel Kiper > Nit, please do not leave empty lines between the tags. I will fix it before committing. > Signed-off-by: Glenn Washburn Otherwise Reviewed-by: Daniel Kiper Daniel ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCHv2] grub-install: Add backup and restore
On Mon, Dec 07, 2020 at 12:37:28PM +, Dimitri John Ledkov wrote: > Refactor clean_grub_dir to create a backup of all the files, instead > of just irrevocably removing them as the first action. If available, > register on_exit handle to restore the backup if any errors occur, or > remove the backup if everything was successful. If on_exit is not > available, the backup remains on disk for manual recovery. Reviewed-by: Colin Watson -- Colin Watson (he/him) [cjwat...@debian.org] ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCHv2] grub-install: Add backup and restore
Refactor clean_grub_dir to create a backup of all the files, instead
of just irrevocably removing them as the first action. If available,
register on_exit handle to restore the backup if any errors occur, or
remove the backup if everything was successful. If on_exit is not
available, the backup remains on disk for manual recovery.
This allows safer upgrades of MBR & modules, such that
modules/images/fonts/translations are consistent with MBR in case of
errors. For example accidental grub-install /dev/non-existent-disk
currently clobbers and upgrades modules in /boot/grub, despite not
actually updating any MBR. This increases peak disk-usage slightly, by
requiring temporarily twice the disk space to complete grub-install.
Also add modinfo.sh to the cleanup/backup/restore codepath, to ensure
it is also cleaned / backed up / restored.
Signed-off-by: Dimitri John Ledkov
---
Changes since v1: as reported on linux-ext4 mailing list and debugged
by Colin Watson, the grub_util_path_concat_ext call was incorrect in
the restore backup case as there was no extention needed. Instead the
call is corrected to just grub_util_path_concat.
This patch is now shipped in Ubuntu & Debian in multiple series. It
would be nice to have this merged upstream, as it greatly improves
grub upgrades and prevents missmatches of MBR & modules.
configure.ac | 2 +-
util/grub-install-common.c | 105 +++--
2 files changed, 91 insertions(+), 16 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7c10a4db7..71cd414c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -419,7 +419,7 @@ else
fi
# Check for functions and headers.
-AC_CHECK_FUNCS(posix_memalign memalign getextmntent)
+AC_CHECK_FUNCS(posix_memalign memalign getextmntent on_exit)
AC_CHECK_HEADERS(sys/param.h sys/mount.h sys/mnttab.h limits.h)
# glibc 2.25 still includes sys/sysmacros.h in sys/types.h but emits
deprecation
diff --git a/util/grub-install-common.c b/util/grub-install-common.c
index 277eaf4e2..74584b92b 100644
--- a/util/grub-install-common.c
+++ b/util/grub-install-common.c
@@ -185,38 +185,113 @@ grub_install_mkdir_p (const char *dst)
free (t);
}
+static int
+strcmp_ext (const char *a, const char *b, const char *ext)
+{
+ char *bsuffix = grub_util_path_concat_ext (1, b, ext);
+ int r = strcmp (a, bsuffix);
+ free (bsuffix);
+ return r;
+}
+
+enum clean_grub_dir_mode
+{
+ CLEAN = 0,
+ CLEAN_BACKUP = 1,
+ CREATE_BACKUP = 2,
+ RESTORE_BACKUP = 3,
+};
+
static void
-clean_grub_dir (const char *di)
+clean_grub_dir_real (const char *di, enum clean_grub_dir_mode mode)
{
grub_util_fd_dir_t d;
grub_util_fd_dirent_t de;
+ char suffix[2] = "";
+
+ if ((mode == CLEAN_BACKUP) || (mode == RESTORE_BACKUP))
+{
+ strcpy (suffix, "-");
+}
d = grub_util_fd_opendir (di);
if (!d)
-grub_util_error (_("cannot open directory `%s': %s"),
-di, grub_util_fd_strerror ());
+{
+ if (mode == CLEAN_BACKUP)
+ return;
+ grub_util_error (_("cannot open directory `%s': %s"),
+ di, grub_util_fd_strerror ());
+}
while ((de = grub_util_fd_readdir (d)))
{
const char *ext = strrchr (de->d_name, '.');
- if ((ext && (strcmp (ext, ".mod") == 0
- || strcmp (ext, ".lst") == 0
- || strcmp (ext, ".img") == 0
- || strcmp (ext, ".mo") == 0)
- && strcmp (de->d_name, "menu.lst") != 0)
- || strcmp (de->d_name, "efiemu32.o") == 0
- || strcmp (de->d_name, "efiemu64.o") == 0)
+ if ((ext && (strcmp_ext (ext, ".mod", suffix) == 0
+ || strcmp_ext (ext, ".lst", suffix) == 0
+ || strcmp_ext (ext, ".img", suffix) == 0
+ || strcmp_ext (ext, ".mo", suffix) == 0)
+ && strcmp_ext (de->d_name, "menu.lst", suffix) != 0)
+ || strcmp_ext (de->d_name, "modinfo.sh", suffix) == 0
+ || strcmp_ext (de->d_name, "efiemu32.o", suffix) == 0
+ || strcmp_ext (de->d_name, "efiemu64.o", suffix) == 0)
{
- char *x = grub_util_path_concat (2, di, de->d_name);
- if (grub_util_unlink (x) < 0)
- grub_util_error (_("cannot delete `%s': %s"), x,
-grub_util_fd_strerror ());
- free (x);
+ char *srcf = grub_util_path_concat (2, di, de->d_name);
+
+ if (mode == CREATE_BACKUP)
+ {
+ char *dstf = grub_util_path_concat_ext (2, di, de->d_name, "-");
+ if (grub_util_rename (srcf, dstf) < 0)
+ grub_util_error (_("cannot backup `%s': %s"), srcf,
+grub_util_fd_strerror ());
+ free (dstf);
+ }
+ else if (mode == RESTORE_BACKUP)
+ {
+ char *dstf = grub_util_path_concat (2, di, de->d_name);
+ dstf[strlen (dstf) - 1] = 0;
+ if (grub_util_rename (srcf, dstf) < 0)
+
