Re: [PATCH v7 03/17] luks2: Remove unused argument in grub_error

2020-12-08 Thread Glenn Washburn 
On Mon, 7 Dec 2020 20:52:44 +0100
Daniel Kiper  wrote:

> 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

This is annoying to do with how my git is setup, which is to
automatically append my SOB when doing format-patch.  Do you just do
SOBs and RBs by hand, or have a way to do this more automatically?

Glenn

___
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

2020-12-08 Thread Glenn Washburn 
On Tue, 8 Dec 2020 07:38:31 +0100
Patrick Steinhardt  wrote:

> 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

On third thought, while I like "id" better because its really a generic
id, we use it as an index into the segments and keyslots bitfield of
the grub_luks2_digest_t struct. So I'm going to I'm going to choose idx
instead of id.  If the implementation changes and we choose to support
arbitrary object names we can change the member name to something more
generic.

Glenn

___
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

2020-12-08 Thread Glenn Washburn 
On Mon, 7 Dec 2020 21:15:33 +0100
Daniel Kiper  wrote:

> 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
> > (&d->json_slot_key, &digest, NULL) || grub_json_getchild (&digest,
> > &digest, 0) || luks2_parse_digest (d, &digest))
> > -   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?

Ugh, yes.

> >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.

Hmm, yes, I'm fairly certain the whole patch series builds, which is
why I didn't catch this.  Some changes got applied to the wrong
patches, which is why this happened, I think.

> >/* Get segment that matches the digest. */
> >if (grub_json_getvalue (&segments, 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 (&digest, 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

Glenn

___
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

2020-12-08 Thread Patrick Steinhardt 
On Tue, Dec 08, 2020 at 11:45:37AM -0600, Glenn Washburn wrote:
> On Mon, 7 Dec 2020 20:52:44 +0100
> Daniel Kiper  wrote:
> 
> > 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
> 
> This is annoying to do with how my git is setup, which is to
> automatically append my SOB when doing format-patch.  Do you just do
> SOBs and RBs by hand, or have a way to do this more automatically?
> 
> Glenn

You can for example just use `git commit --signoff` when creating the
commit. There's also `git rebase --signoff` to add SOBs for rebased
patches if they don't have one yet. If RBs come in during review, I
typically just `git rebase --interactive` and `r` commits to add the
trailing RBs.

Patrick


signature.asc
Description: PGP signature
___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 00/18] Cryptodisk fixes for v2.06 redux

2020-12-08 Thread Glenn Washburn 
This patch series, fixes issues with v7 series. Some of those patches had
changes applied to the wrong patch. The added structure member was renamed
again, this time to idx. And there is an added patch that renames some json
index variables to note that they are such. 

Glenn

Glenn Washburn (18):
  disk: Rename grub_disk_get_size to grub_disk_native_sectors
  misc: Add parentheses around ALIGN_UP and ALIGN_DOWN arguments
  whitespace: convert 8 spaces to tabs
  luks2: Remove unused argument in grub_error
  luks2: Make sure all fields of output argument in luks2_parse_digest()
are written to
  luks2: Add idx member to struct grub_luks2_keyslot/segment/digest
  luks2: Use more intuitive object name instead of json index in user
messages
  luks2: Rename json index variables to names that they are obviously
json indexes
  luks2: Add string "index" to user strings using a json index.
  cryptodisk: Add macro GRUB_TYPE_BITS() to replace some literals
  cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals
  luks2: grub_cryptodisk_t->total_sectors is the max number of device
native sectors
  cryptodisk: Properly handle non-512 byte sized sectors
  luks2: Better error handling when setting up the cryptodisk
  luks2: Error check segment.sector_size
  mips: Enable __clzdi2()
  misc: Add grub_log2ull macro for calculating log base 2 of 64-bit
integers
  luks2: Use grub_log2ull to calculate log_sector_size and improve
readability

 grub-core/disk/cryptodisk.c|  64 ++
 grub-core/disk/diskfilter.c|  12 +-
 grub-core/disk/dmraid_nvidia.c |   2 +-
 grub-core/disk/efi/efidisk.c   |   2 +-
 grub-core/disk/geli.c  |   6 +-
 grub-core/disk/ldm.c   |   4 +-
 grub-core/disk/luks.c  |   7 +-
 grub-core/disk/luks2.c | 184 ++---
 grub-core/disk/mdraid1x_linux.c|   2 +-
 grub-core/disk/mdraid_linux.c  |   2 +-
 grub-core/fs/cbfs.c|  16 +--
 grub-core/fs/nilfs2.c  |   2 +-
 grub-core/fs/zfs/zfs.c |   4 +-
 grub-core/kern/compiler-rt.c   |   2 +-
 grub-core/kern/disk.c  |   2 +-
 grub-core/kern/mips/arc/init.c |   2 +-
 grub-core/normal/misc.c|   6 +-
 grub-core/osdep/windows/platform.c |   2 +-
 include/grub/compiler-rt.h |   2 +-
 include/grub/cryptodisk.h  |   8 +-
 include/grub/disk.h|  21 +++-
 include/grub/misc.h|   7 +-
 include/grub/types.h   |   9 ++
 util/grub-install.c|   2 +-
 util/grub-probe.c  |   2 +-
 25 files changed, 261 insertions(+), 111 deletions(-)

Range-diff against v7:
 1:  a80929b15 !  1:  f4062e4f6 disk: Rename grub_disk_get_size to 
grub_disk_native_sectors
@@ Commit message
 size. Rename to something more appropriate.
 
 Suggested-by: Daniel Kiper 
+Reviewed-by: Daniel Kiper 
+Reviewed-by: Patrick Steinhardt 
 
  ## grub-core/disk/diskfilter.c ##
 @@ grub-core/disk/diskfilter.c: scan_disk_partition_iter (grub_disk_t 
disk, grub_partition_t p, void *data)
 2:  d4b462fd3 !  2:  17b1ef195 misc: Add parentheses around ALIGN_UP and 
ALIGN_DOWN arguments
@@ Commit message
 This ensures that expected order of operations is preserved when 
arguments
 are expressions.
 
+Reviewed-by: Daniel Kiper 
+Reviewed-by: Patrick Steinhardt 
+
  ## include/grub/misc.h ##
 @@
  #include 
 -:  - >  3:  eb8625d9f whitespace: convert 8 spaces to tabs
 3:  d1a23ff0a !  4:  0f28628e2 luks2: Remove unused argument in grub_error
@@ Commit message
 luks2: Remove unused argument in grub_error
 
 Reviewed-by: Daniel Kiper 
+Reviewed-by: Patrick Steinhardt 
 
  ## grub-core/disk/luks2.c ##
 @@ grub-core/disk/luks2.c: luks2_parse_segment (grub_luks2_segment_t *out, 
const grub_json_t *segment)
 4:  61fcebc1a !  5:  705548bc2 luks2: Make sure all fields of output argument 
in luks2_parse_digest() are written to
@@ Commit message
 Otherwise, the digest could say it belongs to keyslots and segments 
that it
 does not.
 
+Reviewed-by: Daniel Kiper 
+Reviewed-by: Patrick Steinhardt 
+
  ## grub-core/disk/luks2.c ##
 @@ grub-core/disk/luks2.c: luks2_parse_digest (grub_luks2_digest_t *out, 
const grub_json_t *digest)
  return grub_error (GRUB_ERR_BAD_ARGUMENT,
 5:  f6a6befdb !  6:  56d4c397a luks2: Add json_slot_key member to struct 
grub_luks2_keyslot/segment/digest
@@ Metadata
 Author: Glenn Washburn 
 
  ## Commit message ##
-luks2: Add json_slot_key member to struct 
grub_luks2_keyslot/segment/digest
+luks2: Add idx member to struct grub_luks2_keyslot/segment/digest
 
 This allows code using these structs to know the named key associated 
with
 these json data stru

[PATCH v8 01/18] disk: Rename grub_disk_get_size to grub_disk_native_sectors

2020-12-08 Thread Glenn Washburn 
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.

Signed-off-by: Glenn Washburn 
Suggested-by: Daniel Kiper 
Reviewed-by: Daniel Kiper 
Reviewed-by: Patrick Steinhardt 
---
 grub-core/disk/diskfilter.c| 12 ++--
 grub-core/disk/dmraid_nvidia.c |  2 +-
 grub-core/disk/efi/efidisk.c   |  2 +-
 grub-core/disk/geli.c  |  6 +++---
 grub-core/disk/ldm.c   |  4 ++--
 grub-core/disk/luks.c  |  2 +-
 grub-core/disk/mdraid1x_linux.c|  2 +-
 grub-core/disk/mdraid_linux.c  |  2 +-
 grub-core/fs/cbfs.c| 16 
 grub-core/fs/nilfs2.c  |  2 +-
 grub-core/fs/zfs/zfs.c |  4 ++--
 grub-core/kern/disk.c  |  2 +-
 grub-core/kern/mips/arc/init.c |  2 +-
 grub-core/normal/misc.c|  6 +++---
 grub-core/osdep/windows/platform.c |  2 +-
 include/grub/disk.h|  4 ++--
 util/grub-install.c|  2 +-
 util/grub-probe.c  |  2 +-
 18 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/grub-core/disk/diskfilter.c b/grub-core/disk/diskfilter.c
index 86557f923..032011566 100644
--- a/grub-core/disk/diskfilter.c
+++ b/grub-core/disk/diskfilter.c
@@ -148,7 +148,7 @@ scan_disk_partition_iter (grub_disk_t disk, 
grub_partition_t p, void *data)
if (m->disk && m->disk->id == disk->id
&& m->disk->dev->id == disk->dev->id
&& m->part_start == grub_partition_get_start (disk->partition)
-   && m->part_size == grub_disk_get_size (disk))
+   && m->part_size == grub_disk_native_sectors (disk))
  return 0;
 }
 
@@ -1190,13 +1190,13 @@ insert_array (grub_disk_t disk, const struct 
grub_diskfilter_pv_id *id,
 
   grub_dprintf ("diskfilter", "Inserting %s (+%lld,%lld) into %s (%s)\n", 
disk->name,
(unsigned long long) grub_partition_get_start (disk->partition),
-   (unsigned long long) grub_disk_get_size (disk),
+   (unsigned long long) grub_disk_native_sectors (disk),
array->name, diskfilter->name);
 #ifdef GRUB_UTIL
   grub_util_info ("Inserting %s (+%" GRUB_HOST_PRIuLONG_LONG ",%"
  GRUB_HOST_PRIuLONG_LONG ") into %s (%s)\n", disk->name,
  (unsigned long long) grub_partition_get_start 
(disk->partition),
- (unsigned long long) grub_disk_get_size (disk),
+ (unsigned long long) grub_disk_native_sectors (disk),
  array->name, diskfilter->name);
   array->driver = diskfilter;
 #endif
@@ -1210,7 +1210,7 @@ insert_array (grub_disk_t disk, const struct 
grub_diskfilter_pv_id *id,
struct grub_diskfilter_lv *lv;
/* FIXME: Check whether the update time of the superblocks are
   the same.  */
-   if (pv->disk && grub_disk_get_size (disk) >= pv->part_size)
+   if (pv->disk && grub_disk_native_sectors (disk) >= pv->part_size)
  return GRUB_ERR_NONE;
pv->disk = grub_disk_open (disk->name);
if (!pv->disk)
@@ -1219,7 +1219,7 @@ insert_array (grub_disk_t disk, const struct 
grub_diskfilter_pv_id *id,
   raid device, we shouldn't change it.  */
pv->start_sector -= pv->part_start;
pv->part_start = grub_partition_get_start (disk->partition);
-   pv->part_size = grub_disk_get_size (disk);
+   pv->part_size = grub_disk_native_sectors (disk);
 
 #ifdef GRUB_UTIL
{
@@ -1311,7 +1311,7 @@ grub_diskfilter_get_pv_from_disk (grub_disk_t disk,
if (pv->disk && pv->disk->id == disk->id
&& pv->disk->dev->id == disk->dev->id
&& pv->part_start == grub_partition_get_start (disk->partition)
-   && pv->part_size == grub_disk_get_size (disk))
+   && pv->part_size == grub_disk_native_sectors (disk))
  {
if (vg_out)
  *vg_out = vg;
diff --git a/grub-core/disk/dmraid_nvidia.c b/grub-core/disk/dmraid_nvidia.c
index 060279124..4d2fb04d1 100644
--- a/grub-core/disk/dmraid_nvidia.c
+++ b/grub-core/disk/dmraid_nvidia.c
@@ -107,7 +107,7 @@ grub_dmraid_nv_detect (grub_disk_t disk,
 /* Skip partition.  */
 return NULL;
 
-  sector = grub_disk_get_size (disk);
+  sector = grub_disk_native_sectors (disk);
   if (sector == GRUB_DISK_SIZE_UNKNOWN)
 /* Not raid.  */
 return NULL;
diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index 9e20af70e..f077b5f55 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -867,7 +867,7 @@ grub_efidisk_get_device_name (grub_efi_handle_t *handle)
   if (ctx.hd->partition_start == 0
  && (ctx.hd->partition_size << (parent->log_sector_size
 - GRUB_DISK_SECTOR_BITS))
- == grub_disk_get_size (parent))
+ == grub_disk_native_sectors (parent))

[PATCH v8 07/18] luks2: Use more intuitive object name instead of json index in user messages

2020-12-08 Thread Glenn Washburn 
Use the object 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 | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 67b1823d3..b6930b2f5 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_SIZE, keyslot_idx);
+  return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot 
\"%"PRIuGRUB_UINT64_T"\"", k->idx);
 
   /* Get segment that matches the digest. */
   if (grub_json_getvalue (&segments, 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->idx);
 
   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.idx);
  continue;
}
 
-  grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
+  grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n", 
keyslot.idx);
 
   /* 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.idx, grub_errmsg);
  continue;
}
 
   ret = luks2_verify_key (&digest, 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.idx, 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.idx);
 
   candidate_key_len = keyslot.key_size;
   break;
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 06/18] luks2: Add idx member to struct grub_luks2_keyslot/segment/digest

2020-12-08 Thread Glenn Washburn 
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 local variable in luks2_get_keyslot() which was overloaded to
be used for both keyslot and segment slot keys.

Signed-off-by: Glenn Washburn 
---
 grub-core/disk/luks2.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 9b19d35c1..67b1823d3 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -65,6 +65,8 @@ typedef struct grub_luks2_header grub_luks2_header_t;
 
 struct grub_luks2_keyslot
 {
+  /* The integer key to the associative array of keyslots */
+  grub_uint64_t idx;
   grub_int64_t key_size;
   grub_int64_t priority;
   struct
@@ -103,6 +105,7 @@ typedef struct grub_luks2_keyslot grub_luks2_keyslot_t;
 
 struct grub_luks2_segment
 {
+  grub_uint64_t idx;
   grub_uint64_t offset;
   const char   *size;
   const char   *encryption;
@@ -112,6 +115,7 @@ typedef struct grub_luks2_segment grub_luks2_segment_t;
 
 struct grub_luks2_digest
 {
+  grub_uint64_t idx;
   /* Both keyslots and segments are interpreted as bitfields here */
   grub_uint64_tkeyslots;
   grub_uint64_tsegments;
@@ -261,12 +265,11 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
 {
   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
   grub_size_t i, size;
-  grub_uint64_t idx;
 
   /* Get nth keyslot */
   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
   grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
-  grub_json_getuint64 (&idx, &keyslot, NULL) ||
+  grub_json_getuint64 (&k->idx, &keyslot, NULL) ||
   grub_json_getchild (&keyslot, &keyslot, 0) ||
   luks2_parse_keyslot (k, &keyslot))
 return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
%"PRIuGRUB_SIZE, keyslot_idx);
@@ -278,11 +281,12 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
   for (i = 0; i < size; i++)
 {
   if (grub_json_getchild (&digest, &digests, i) ||
+ grub_json_getuint64 (&d->idx, &digest, NULL) ||
  grub_json_getchild (&digest, &digest, 0) ||
  luks2_parse_digest (d, &digest))
return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest 
%"PRIuGRUB_SIZE, i);
 
-  if ((d->keyslots & (1 << idx)))
+  if ((d->keyslots & (1 << k->idx)))
break;
 }
   if (i == size)
@@ -295,12 +299,12 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
   for (i = 0; i < size; i++)
 {
   if (grub_json_getchild (&segment, &segments, i) ||
- grub_json_getuint64 (&idx, &segment, NULL) ||
+ grub_json_getuint64 (&s->idx, &segment, NULL) ||
  grub_json_getchild (&segment, &segment, 0) ||
  luks2_parse_segment (s, &segment))
return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, i);
 
-  if ((d->segments & (1 << idx)))
+  if ((d->segments & (1 << s->idx)))
break;
 }
   if (i == size)
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 03/18] whitespace: convert 8 spaces to tabs

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
Reviewed-by: Patrick Steinhardt 
---
 grub-core/disk/luks2.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index d96764a02..66e3b8798 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -276,8 +276,8 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
   for (i = 0; i < size; i++)
 {
   if (grub_json_getchild (&digest, &digests, i) ||
-  grub_json_getchild (&digest, &digest, 0) ||
-  luks2_parse_digest (d, &digest))
+ grub_json_getchild (&digest, &digest, 0) ||
+ luks2_parse_digest (d, &digest))
return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest 
%"PRIuGRUB_SIZE, i);
 
   if ((d->keyslots & (1 << idx)))
@@ -295,7 +295,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
   if (grub_json_getchild (&segment, &segments, i) ||
  grub_json_getuint64 (&idx, &segment, NULL) ||
  grub_json_getchild (&segment, &segment, 0) ||
-  luks2_parse_segment (s, &segment))
+ luks2_parse_segment (s, &segment))
return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, i);
 
   if ((d->segments & (1 << idx)))
@@ -600,7 +600,7 @@ luks2_recover_key (grub_disk_t source,
{
  grub_dprintf ("luks2", "Ignoring keyslot %"PRIuGRUB_SIZE" due to 
priority\n", i);
  continue;
-}
+   }
 
   grub_dprintf ("luks2", "Trying keyslot %"PRIuGRUB_SIZE"\n", i);
 
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 10/18] cryptodisk: Add macro GRUB_TYPE_BITS() to replace some literals

2020-12-08 Thread Glenn Washburn 
The new macro GRUB_TYPE_BITS(type) returns the number of bits allocated for
type.

Signed-off-by: Glenn Washburn 
---
 grub-core/disk/cryptodisk.c | 7 ---
 include/grub/types.h| 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 473c93976..0e955a020 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -281,20 +281,21 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
  }
  break;
case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
- iv[1] = grub_cpu_to_le32 (sector >> 32);
+ 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);
  break;
case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
- iv[1] = grub_cpu_to_le32 (sector >> (32 - dev->log_sector_size));
+ 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);
  break;
case GRUB_CRYPTODISK_MODE_IV_BENBI:
  {
grub_uint64_t num = (sector << dev->benbi_log) + 1;
-   iv[sz - 2] = grub_cpu_to_be32 (num >> 32);
+   iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BITS (iv[0]));
iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
  }
  break;
diff --git a/include/grub/types.h b/include/grub/types.h
index f22055f98..9989e3a16 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -80,6 +80,8 @@
 # define GRUB_CHAR_BIT __CHAR_BIT__
 #endif
 
+#define GRUB_TYPE_BITS(type) (sizeof(type) * GRUB_CHAR_BIT)
+
 /* Define various wide integers.  */
 typedef signed chargrub_int8_t;
 typedef short  grub_int16_t;
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 04/18] luks2: Remove unused argument in grub_error

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
Reviewed-by: Patrick Steinhardt 
---
 grub-core/disk/luks2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 66e3b8798..402926680 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -200,7 +200,7 @@ luks2_parse_segment (grub_luks2_segment_t *out, const 
grub_json_t *segment)
   grub_json_getstring (&out->size, segment, "size") ||
   grub_json_getstring (&out->encryption, segment, "encryption") ||
   grub_json_getint64 (&out->sector_size, segment, "sector_size"))
-return grub_error (GRUB_ERR_BAD_ARGUMENT, "Missing segment parameters", 
type);
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "Missing segment parameters");
 
   return GRUB_ERR_NONE;
 }
@@ -228,7 +228,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const 
grub_json_t *digest)
 
   if (grub_json_getsize (&size, &segments))
 return grub_error (GRUB_ERR_BAD_ARGUMENT,
-  "Digest references no segments", type);
+  "Digest references no segments");
 
   for (i = 0; i < size; i++)
 {
@@ -240,7 +240,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const 
grub_json_t *digest)
 
   if (grub_json_getsize (&size, &keyslots))
 return grub_error (GRUB_ERR_BAD_ARGUMENT,
-  "Digest references no keyslots", type);
+  "Digest references no keyslots");
 
   for (i = 0; i < size; i++)
 {
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 14/18] luks2: Better error handling when setting up the cryptodisk

2020-12-08 Thread Glenn Washburn 
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", verify that the offset is not past the end of disk. Otherwise,
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, but only allow
access up to the end of the source disk.

Signed-off-by: Glenn Washburn 
---
 grub-core/disk/luks2.c | 84 --
 include/grub/disk.h| 17 +
 2 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 9abcb1c94..8cb11e899 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -600,12 +600,26 @@ 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 (json_idx = 0; json_idx < size; json_idx++)
 {
+  typeof(source->total_sectors) max_crypt_sectors = 0;
+
+  grub_errno = GRUB_ERR_NONE;
   ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, json_idx);
   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)
{
@@ -619,11 +633,75 @@ luks2_recover_key (grub_disk_t source,
   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.idx, crypt->offset_sectors,
+max_crypt_sectors);
+ continue;
+   }
+
   if (grub_strcmp (segment.size, "dynamic") == 0)
-   crypt->total_sectors = (grub_disk_get_size (source) >> 
(crypt->log_sector_size - source->log_sector_size))
-  - crypt->offset_sectors;
+   crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
   else
-   crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> 
crypt->log_sector_size;
+   {
+ grub_errno = GRUB_ERR_NONE;
+ /* Convert segment.size to sectors, rounding up to nearest sector */
+ crypt->total_sectors = grub_strtoull (segment.size, NULL, 10);
+ crypt->total_sectors = ALIGN_UP (crypt->total_sectors,
+  1 << crypt->log_sector_size);
+ crypt->total_sectors >>= crypt->log_sector_size;
+
+ if (grub_errno == GRUB_ERR_NONE)
+   ;
+ else if (grub_errno == GRUB_ERR_BAD_NUMBER)
+   {
+ grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
+" \"%s\" is not a parsable number\n",
+segment.idx, segment.size);
+ continue;
+   }
+ else if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
+   {
+ /*
+  * There was an overflow in parsing segment.size, so disk must
+  * be very large or the string is incorrect.
+  */
+ grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT

[PATCH v8 17/18] misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
---
 include/grub/misc.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/grub/misc.h b/include/grub/misc.h
index 780a34e90..73a514eb1 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -482,4 +482,7 @@ void EXPORT_FUNC(grub_real_boot_time) (const char *file,
 #define grub_max(a, b) (((a) > (b)) ? (a) : (b))
 #define grub_min(a, b) (((a) < (b)) ? (a) : (b))
 
+#define grub_log2ull(n) (GRUB_TYPE_BITS (grub_uint64_t) \
+ - __builtin_clzll (n) - 1)
+
 #endif /* ! GRUB_MISC_HEADER */
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 18/18] luks2: Use grub_log2ull to calculate log_sector_size and improve readability

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
---
 grub-core/disk/luks2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index c737797b9..31b51f1d4 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -642,8 +642,7 @@ luks2_recover_key (grub_disk_t source,
 
   /* 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;
+  crypt->log_sector_size = grub_log2ull (segment.sector_size);
   /* Set to the source disk size, which is the maximum we allow. */
   max_crypt_sectors = grub_disk_convert_sector(source,
   source->total_sectors,
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 08/18] luks2: Rename json index variables to names that they are obviously json indexes

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
---
 grub-core/disk/luks2.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index b6930b2f5..7e0419a82 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -261,53 +261,53 @@ luks2_parse_digest (grub_luks2_digest_t *out, const 
grub_json_t *digest)
 
 static grub_err_t
 luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, 
grub_luks2_segment_t *s,
-  const grub_json_t *root, grub_size_t keyslot_idx)
+  const grub_json_t *root, grub_size_t keyslot_json_idx)
 {
   grub_json_t keyslots, keyslot, digests, digest, segments, segment;
-  grub_size_t i, size;
+  grub_size_t json_idx, size;
 
   /* Get nth keyslot */
   if (grub_json_getvalue (&keyslots, root, "keyslots") ||
-  grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
+  grub_json_getchild (&keyslot, &keyslots, keyslot_json_idx) ||
   grub_json_getuint64 (&k->idx, &keyslot, NULL) ||
   grub_json_getchild (&keyslot, &keyslot, 0) ||
   luks2_parse_keyslot (k, &keyslot))
-return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
%"PRIuGRUB_SIZE, keyslot_idx);
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
%"PRIuGRUB_SIZE, keyslot_json_idx);
 
   /* Get digest that matches the keyslot. */
   if (grub_json_getvalue (&digests, root, "digests") ||
   grub_json_getsize (&size, &digests))
 return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get digests");
-  for (i = 0; i < size; i++)
+  for (json_idx = 0; json_idx < size; json_idx++)
 {
-  if (grub_json_getchild (&digest, &digests, i) ||
+  if (grub_json_getchild (&digest, &digests, json_idx) ||
  grub_json_getuint64 (&d->idx, &digest, NULL) ||
  grub_json_getchild (&digest, &digest, 0) ||
  luks2_parse_digest (d, &digest))
-   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 
%"PRIuGRUB_SIZE, json_idx);
 
   if ((d->keyslots & (1 << k->idx)))
break;
 }
-  if (i == size)
+  if (json_idx == size)
   return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for keyslot 
\"%"PRIuGRUB_UINT64_T"\"", k->idx);
 
   /* Get segment that matches the digest. */
   if (grub_json_getvalue (&segments, root, "segments") ||
   grub_json_getsize (&size, &segments))
 return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not get segments");
-  for (i = 0; i < size; i++)
+  for (json_idx = 0; json_idx < size; json_idx++)
 {
-  if (grub_json_getchild (&segment, &segments, i) ||
+  if (grub_json_getchild (&segment, &segments, json_idx) ||
  grub_json_getuint64 (&s->idx, &segment, NULL) ||
  grub_json_getchild (&segment, &segment, 0) ||
  luks2_parse_segment (s, &segment))
-   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, i);
+   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, json_idx);
 
   if ((d->segments & (1 << s->idx)))
break;
 }
-  if (i == size)
+  if (json_idx == size)
 return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for digest 
\"%"PRIuGRUB_UINT64_T"\"", d->idx);
 
   return GRUB_ERR_NONE;
@@ -542,7 +542,7 @@ luks2_recover_key (grub_disk_t source,
   grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
   char passphrase[MAX_PASSPHRASE], cipher[32];
   char *json_header = NULL, *part = NULL, *ptr;
-  grub_size_t candidate_key_len = 0, i, size;
+  grub_size_t candidate_key_len = 0, json_idx, size;
   grub_luks2_header_t header;
   grub_luks2_keyslot_t keyslot;
   grub_luks2_digest_t digest;
@@ -596,9 +596,9 @@ luks2_recover_key (grub_disk_t source,
 }
 
   /* Try all keyslot */
-  for (i = 0; i < size; i++)
+  for (json_idx = 0; json_idx < size; json_idx++)
 {
-  ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, i);
+  ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, json_idx);
   if (ret)
goto err;
 
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 11/18] cryptodisk: Add macros GRUB_TYPE_U_MAX/MIN(type) to replace literals

2020-12-08 Thread Glenn Washburn 
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 
---
 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..b36b26a79 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.
+ */
+#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)]
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 05/18] luks2: Make sure all fields of output argument in luks2_parse_digest() are written to

2020-12-08 Thread Glenn Washburn 
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 
Reviewed-by: Patrick Steinhardt 
---
 grub-core/disk/luks2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 402926680..9b19d35c1 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -230,6 +230,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const 
grub_json_t *digest)
 return grub_error (GRUB_ERR_BAD_ARGUMENT,
   "Digest references no segments");
 
+  out->segments = 0;
   for (i = 0; i < size; i++)
 {
   if (grub_json_getchild (&o, &segments, i) ||
@@ -242,6 +243,7 @@ luks2_parse_digest (grub_luks2_digest_t *out, const 
grub_json_t *digest)
 return grub_error (GRUB_ERR_BAD_ARGUMENT,
   "Digest references no keyslots");
 
+  out->keyslots = 0;
   for (i = 0; i < size; i++)
 {
   if (grub_json_getchild (&o, &keyslots, i) ||
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 15/18] luks2: Error check segment.sector_size

2020-12-08 Thread Glenn Washburn 
Signed-off-by: Glenn Washburn 
---
 grub-core/disk/luks2.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 8cb11e899..c737797b9 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -629,6 +629,17 @@ luks2_recover_key (grub_disk_t source,
 
   grub_dprintf ("luks2", "Trying keyslot \"%"PRIuGRUB_UINT64_T"\"\n", 
keyslot.idx);
 
+  /* Sector size should be one of 512, 1024, 2048, or 4096. */
+  if (!(segment.sector_size == 512 || segment.sector_size == 1024 ||
+   segment.sector_size == 2048 || segment.sector_size == 4096))
+   {
+ grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" sector"
+" size %"PRIuGRUB_UINT64_T" is not one of"
+" 512, 1024, 2048, or 4096\n",
+segment.idx, segment.sector_size);
+ continue;
+   }
+
   /* 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
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 16/18] mips: Enable __clzdi2()

2020-12-08 Thread Glenn Washburn 
This patch is similiar to commit 9dab2f51e (sparc: Enable __clzsi2() and
__clzdi2()) but for MIPS target and __clzdi2 only, __clzsi2 was already
enabled.

Signed-off-by: Glenn Washburn 
Suggested-by: Daniel Kiper 
---
 grub-core/kern/compiler-rt.c | 2 +-
 include/grub/compiler-rt.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/kern/compiler-rt.c b/grub-core/kern/compiler-rt.c
index a464200c6..2057c2e0c 100644
--- a/grub-core/kern/compiler-rt.c
+++ b/grub-core/kern/compiler-rt.c
@@ -448,7 +448,7 @@ __clzsi2 (grub_uint32_t val)
 }
 #endif
 
-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 __clzdi2 (grub_uint64_t val)
 {
diff --git a/include/grub/compiler-rt.h b/include/grub/compiler-rt.h
index 7591980b4..17828b322 100644
--- a/include/grub/compiler-rt.h
+++ b/include/grub/compiler-rt.h
@@ -115,7 +115,7 @@ int
 EXPORT_FUNC (__clzsi2) (grub_uint32_t val);
 #endif
 
-#if defined(__riscv) || defined(__sparc__)
+#if defined(__mips__) || defined(__riscv) || defined(__sparc__)
 int
 EXPORT_FUNC (__clzdi2) (grub_uint64_t val);
 #endif
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 02/18] misc: Add parentheses around ALIGN_UP and ALIGN_DOWN arguments

2020-12-08 Thread Glenn Washburn 
This ensures that expected order of operations is preserved when arguments
are expressions.

Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
Reviewed-by: Patrick Steinhardt 
---
 include/grub/misc.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/grub/misc.h b/include/grub/misc.h
index b7ca6dd58..780a34e90 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -28,10 +28,10 @@
 #include 
 
 #define ALIGN_UP(addr, align) \
-   ((addr + (typeof (addr)) align - 1) & ~((typeof (addr)) align - 1))
+   (((addr) + (typeof (addr)) (align) - 1) & ~((typeof (addr)) (align) - 
1))
 #define ALIGN_UP_OVERHEAD(addr, align) ((-(addr)) & ((typeof (addr)) (align) - 
1))
 #define ALIGN_DOWN(addr, align) \
-   ((addr) & ~((typeof (addr)) align - 1))
+   ((addr) & ~((typeof (addr)) (align) - 1))
 #define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))
 #define COMPILE_TIME_ASSERT(cond) switch (0) { case 1: case !(cond): ; }
 
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 09/18] luks2: Add string "index" to user strings using a json index.

2020-12-08 Thread Glenn Washburn 
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 
---
 grub-core/disk/luks2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 7e0419a82..ebf03e77b 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -272,7 +272,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
   grub_json_getuint64 (&k->idx, &keyslot, NULL) ||
   grub_json_getchild (&keyslot, &keyslot, 0) ||
   luks2_parse_keyslot (k, &keyslot))
-return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot 
%"PRIuGRUB_SIZE, keyslot_json_idx);
+return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse keyslot index 
%"PRIuGRUB_SIZE, keyslot_json_idx);
 
   /* Get digest that matches the keyslot. */
   if (grub_json_getvalue (&digests, root, "digests") ||
@@ -284,7 +284,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
  grub_json_getuint64 (&d->idx, &digest, NULL) ||
  grub_json_getchild (&digest, &digest, 0) ||
  luks2_parse_digest (d, &digest))
-   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest 
%"PRIuGRUB_SIZE, json_idx);
+   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse digest index 
%"PRIuGRUB_SIZE, json_idx);
 
   if ((d->keyslots & (1 << k->idx)))
break;
@@ -302,7 +302,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, 
grub_luks2_digest_t *d, grub_luks2_s
  grub_json_getuint64 (&s->idx, &segment, NULL) ||
  grub_json_getchild (&segment, &segment, 0) ||
  luks2_parse_segment (s, &segment))
-   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
%"PRIuGRUB_SIZE, json_idx);
+   return grub_error (GRUB_ERR_BAD_ARGUMENT, "Could not parse segment 
index %"PRIuGRUB_SIZE, json_idx);
 
   if ((d->segments & (1 << s->idx)))
break;
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v8 13/18] cryptodisk: Properly handle non-512 byte sized sectors

2020-12-08 Thread Glenn Washburn 
By default, dm-crypt internally uses an IV that corresponds to 512-byte
sectors, even when a larger sector size is specified. What this means is
that when using a larger sector size, the IV is incremented every sector.
However, the amount the IV is incremented is the number of 512 byte blocks
in a sector (ie 8 for 4K sectors). Confusingly the IV does not corespond to
the number of, for example, 4K sectors. So each 512 byte cipher block in a
sector will be encrypted with the same IV and the IV will be incremented
afterwards by the number of 512 byte cipher blocks in the sector.

There are some encryption utilities which do it the intuitive way and have
the IV equal to the sector number regardless of sector size (ie. the fifth
sector would have an IV of 4 for each cipher block). And this is supported
by dm-crypt with the iv_large_sectors option and also cryptsetup as of 2.3.3
with the --iv-large-sectors, though not with LUKS headers (only with --type
plain). However, support for this has not been included as grub does not
support plain devices right now.

One gotcha here is that the encrypted split keys are encrypted with a hard-
coded 512-byte sector size. So even if your data is encrypted with 4K sector
sizes, the split key encrypted area must be decrypted with a block size of
512 (ie the IV increments every 512 bytes). This made these changes less
aestetically pleasing than desired.

Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
---
 grub-core/disk/cryptodisk.c | 55 +++--
 grub-core/disk/luks.c   |  5 ++--
 grub-core/disk/luks2.c  |  7 -
 include/grub/cryptodisk.h   |  8 +-
 4 files changed, 50 insertions(+), 25 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5aa0c4720..b62835acc 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -224,7 +224,8 @@ lrw_xor (const struct lrw_sector *sec,
 static gcry_err_code_t
 grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
   grub_uint8_t * data, grub_size_t len,
-  grub_disk_addr_t sector, int do_encrypt)
+  grub_disk_addr_t sector, grub_size_t log_sector_size,
+  int do_encrypt)
 {
   grub_size_t i;
   gcry_err_code_t err;
@@ -237,7 +238,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
 return (do_encrypt ? grub_crypto_ecb_encrypt (dev->cipher, data, data, len)
: grub_crypto_ecb_decrypt (dev->cipher, data, data, len));
 
-  for (i = 0; i < len; i += (1U << dev->log_sector_size))
+  for (i = 0; i < len; i += (1U << log_sector_size))
 {
   grub_size_t sz = ((dev->cipher->cipher->blocksize
 + sizeof (grub_uint32_t) - 1)
@@ -270,7 +271,7 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
if (!ctx)
  return GPG_ERR_OUT_OF_MEMORY;
 
-   tmp = grub_cpu_to_le64 (sector << dev->log_sector_size);
+   tmp = grub_cpu_to_le64 (sector << log_sector_size);
dev->iv_hash->init (ctx);
dev->iv_hash->write (ctx, dev->iv_prefix, dev->iv_prefix_len);
dev->iv_hash->write (ctx, &tmp, sizeof (tmp));
@@ -281,15 +282,27 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
  }
  break;
case GRUB_CRYPTODISK_MODE_IV_PLAIN64:
- 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 & GRUB_TYPE_U_MAX (iv[0]));
+ /*
+  * The IV is a 32 or 64 bit value of the dm-crypt native sector
+  * number. If using 32 bit IV mode, zero out the most significant
+  * 32 bits.
+  */
+ {
+   grub_uint64_t iv64;
+
+   iv64 = grub_cpu_to_le64 (sector << (log_sector_size
+- 
GRUB_CRYPTODISK_IV_LOG_SIZE));
+   grub_set_unaligned64 (iv, iv64);
+   if (dev->mode_iv == GRUB_CRYPTODISK_MODE_IV_PLAIN)
+ iv[1] = 0;
+ }
  break;
case GRUB_CRYPTODISK_MODE_IV_BYTECOUNT64:
+ /* The IV is the 64 bit byte offset of the sector. */
  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)
+  - log_sector_size));
+ iv[0] = grub_cpu_to_le32 ((sector << log_sector_size)
& GRUB_TYPE_U_MAX (iv[0]));
  break;
case GRUB_CRYPTODISK_MODE_IV_BENBI:
@@ -312,10 +325,10 @@ grub_cryptodisk_endecrypt (struct grub_cryptodisk *dev,
case GRUB_CRYPTODISK_MODE_CBC:
  if (do_encrypt)
err = grub_crypto_cbc_encrypt (dev->cipher, data + i, data + i,
-

[PATCH v8 12/18] luks2: grub_cryptodisk_t->total_sectors is the max number of device native sectors

2020-12-08 Thread Glenn Washburn 
We need to convert the sectors from the size of the underlying device to the
cryptodisk sector size; segment.size is in bytes which need to be converted
to cryptodisk sectors as well.

Also, removed an empty statement.

Signed-off-by: Glenn Washburn 
Reviewed-by: Daniel Kiper 
---
 grub-core/disk/luks2.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index ebf03e77b..429a4cfa1 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -429,7 +429,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
   grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
   grub_uint8_t *split_key = NULL;
   grub_size_t saltlen = sizeof (salt);
-  char cipher[32], *p;;
+  char cipher[32], *p;
   const gcry_md_spec_t *hash;
   gcry_err_code_t gcry_ret;
   grub_err_t ret;
@@ -615,9 +615,10 @@ luks2_recover_key (grub_disk_t source,
   crypt->log_sector_size = sizeof (unsigned int) * 8
- __builtin_clz ((unsigned int) segment.sector_size) - 1;
   if (grub_strcmp (segment.size, "dynamic") == 0)
-   crypt->total_sectors = grub_disk_get_size (source) - 
crypt->offset_sectors;
+   crypt->total_sectors = (grub_disk_get_size (source) >> 
(crypt->log_sector_size - source->log_sector_size))
+  - crypt->offset_sectors;
   else
-   crypt->total_sectors = grub_strtoull (segment.size, NULL, 10);
+   crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> 
crypt->log_sector_size;
 
   ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
   (const grub_uint8_t *) passphrase, grub_strlen 
(passphrase));
-- 
2.27.0


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH] efi: Copy load_options into environment

2020-12-08 Thread Jordan Webb 
When GRUB is loaded as an EFI application, it will copy the EFI
LoadOptions into an environment variable called "efi_load_options" and
export it.

My use case for this is to pass along arguments generated by the
Raspberry Pi's internal bootloader from U-Boot to GRUB to a Linux
kernel, but I think it's something that could be generally useful to
other people doing interesting things in EFI environments.
---
 grub-core/kern/efi/init.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/grub-core/kern/efi/init.c b/grub-core/kern/efi/init.c
index 2c31847bf..29281c3c5 100644
--- a/grub-core/kern/efi/init.c
+++ b/grub-core/kern/efi/init.c
@@ -17,6 +17,7 @@
  *  along with GRUB.  If not, see .
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -31,6 +32,10 @@ grub_addr_t grub_modbase;
 void
 grub_efi_init (void)
 {
+  grub_efi_loaded_image_t *image = NULL;
+  int len;
+  char *load_options;
+
   grub_modbase = grub_efi_modules_addr ();
   /* First of all, initialize the console so that GRUB can display
  messages.  */
@@ -43,6 +48,14 @@ grub_efi_init (void)
  0, 0, 0, NULL);
 
   grub_efidisk_init ();
+
+  /* Copy EFI load options into environment */
+  image = grub_efi_get_loaded_image (grub_efi_image_handle);
+  len = image->load_options_size / sizeof (grub_efi_char16_t);
+  load_options = grub_malloc (len * sizeof (char));
+  *grub_utf16_to_utf8 ((grub_uint8_t *) load_options, image->load_options, 
len) = '\0';
+  grub_env_set("efi_load_options", load_options);
+  grub_env_export("efi_load_options");
 }
 
 void (*grub_efi_net_config) (grub_efi_handle_t hnd,
-- 
2.24.3 (Apple Git-128)



___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCHv2] grub-install: Add backup and restore

2020-12-08 Thread Michael Chang via Grub-devel 
On Tue, Dec 08, 2020 at 05:58:40AM +, Dimitri John Ledkov wrote:
> 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.

My concern is exactly that these are not upstreamed yet so accepting the
patch would be premature. Either it has to be adapted to satisfy the
expected behavior of existing codebase or wait until those dependent
patches gets merged.

Besides, other architecture like openfirmware would update nvram in a
similar fashion to uefi but cannot be covered by the aforementioned
efivars enhacement patch.

> 
> 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.

Installing signed grub can happen in parallel with any unsigned one
installed by grub-install as long as they didn't overlap in the position
on EFI System partition. For that matters the efibootmgr is used to
manage them.

The signed grub is so far not controlled by grub-install, so should be
disregared at this moment. The entire backup/recovery should be done
only for those made aware by grub-install.

> 
> 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.

IMHO the hook should be unregistered right after the state is considered
complete for new image and modules, or it would be triggered
inadvertently if any error comes after that and hence render the system
incompete again by restoring old backup.

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.
> > >
> > >

Re: [SPECIFICATION RFC] The firmware and bootloader log specification

2020-12-08 Thread Frank Rowand 
On 12/4/20 7:23 AM, 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 forwarded this to Sean.

-Frank

> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: 
> http://events17.linuxfoundation.org/sites/events/files/slides/2016-10-12%20-%20ELCE%20-%20Shared%20Logging%20-%20Part%20Deux.pdf


___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel