Re: [PATCH v6 04/12] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt

2020-12-03 Thread Glenn Washburn 
On Wed, 2 Dec 2020 18:37:42 +0100
Daniel Kiper  wrote:

> On Fri, Nov 27, 2020 at 03:03:36AM -0600, Glenn Washburn wrote:
> > This should improve readability of code by providing clues as to
> > what the value represents. The new macro GRUB_TYPE_BITS(type)
> > returns the number of bits allocated for type. Also add
> > GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> > unsigned number with size of type.
> 
> Two separate patches please...

Ok.

> > Signed-off-by: Glenn Washburn 
> > ---
> >  grub-core/disk/cryptodisk.c | 13 +++--
> >  include/grub/types.h|  5 +
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 473c93976..31b73c535 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -284,22 +284,23 @@ grub_cryptodisk_endecrypt (struct
> > grub_cryptodisk *dev, iv[1] = grub_cpu_to_le32 (sector >> 32);
> >   /* 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 >> (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);
> > +   & 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 >> 32);
> > -   iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> > +   iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BITS
> > (iv[0]));
> > +   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 f22055f98..29d807f71 100644
> > --- a/include/grub/types.h
> > +++ b/include/grub/types.h
> > @@ -72,6 +72,8 @@
> >  # endif
> >  #endif
> >
> > +#define GRUB_TYPE_BITS(type) (sizeof(type) * GRUB_CHAR_BIT)
> 
> This macro should go below "#ifndef __CHAR_BIT__ ...".

Ack.

> >  #ifndef __CHAR_BIT__
> >  # error __CHAR_BIT__ is not defined
> >  #elif __CHAR_BIT__ != 8
> > @@ -159,6 +161,9 @@ typedef grub_int32_tgrub_ssize_t;
> >  #endif
> >  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
> >
> > +#define GRUB_TYPE_U_MAX(type) ((typeof (1ULL))((typeof
> > (type))(~0)))
> 
> (typeof (1ULL)) == (unsigned long long)?

I'll change it.

> > +#define GRUB_TYPE_U_MIN(type) 0ULL
> 
> I am not sure why you cast everything to ULL/unsigned long long.
> Should not the final cast be to (typeof (type))?

I think that to as great as an extent as possible macros should behave
like functions.  So in this case, its "return type" should not change
depending on the input arguments.  This can be confusing and the source
of errors.  Sure, you can say that the user of the macro can just do
the cast herself, but why add potentially unnecessary steps.

Also, I wanted to unsure that the result was an unsigned type,
regardless of signedness of input type. And using the largest unsigned
type seemed the best because I don't know how to take an arbitrary
integer type can only convert its signedness.

But at this point, I really don't care.  Tell me what you want it to be
and I'll make it so.

Glenn

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

Re: [PATCH v6 05/12] luks2: grub_cryptodisk_t->total_sectors is the max number of device native sectors

2020-12-03 Thread Glenn Washburn 
On Wed, 2 Dec 2020 18:56:46 +0100
Daniel Kiper  wrote:

> On Fri, Nov 27, 2020 at 03:03:37AM -0600, Glenn Washburn wrote:
> > 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. And
> > counter-intuitively, grub_disk_get_size returns the total number of
> > device native sectors.
> 
> Should not we change the name to, e.g., grub_disk_native_sectors()
> then? Could you do that?

Sure, I can do that.

> > Also, removed an empty statement.
> >
> > 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: [PATCH v6 01/12] luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest

2020-12-03 Thread Daniel Kiper 
On Thu, Dec 03, 2020 at 01:23:17AM -0600, Glenn Washburn wrote:
> On Wed, 2 Dec 2020 18:01:47 +0100
> Daniel Kiper  wrote:
>
> > On Fri, Nov 27, 2020 at 03:03:33AM -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
> >
> > I prefer if you add "()" to the function names, i.e.
> > luks2_get_keyslot(), in the comments and commit messages. This way it
> > is easier to understand what you mean.
>
> Ok.
>
> > > for both keyslot and segment slot keys.
> > >
> > > Signed-off-by: Glenn Washburn 
> > > ---
> > >  grub-core/disk/luks2.c | 13 -
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index d96764a02..ab2c31dcd 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -65,6 +65,7 @@ typedef struct grub_luks2_header
> > > grub_luks2_header_t;
> > >
> > >  struct grub_luks2_keyslot
> > >  {
> > > +  grub_uint64_t slot_key;
> >
> > Could you be more consistent and use keyslot instead of slot_key here?
>
> I intentionally chose not to use keyslot because I thought it was
> confusing.  slot_key is not a keyslot in the sense of a slot for a
> cryptographic key or key material as in the usage keyslot in the struct
> named "grub_luks2_keyslot".  Its the key value of a "slot" in a json
> associative array which is modeling a sparse array.  Perhaps just "key"
> might be more to your liking?

IMO "key" is more confusing in this context and it would require at
least a comment to clarify what it means. Is not json_slot_key better
then? Probably with some comment what is it too...

> > >grub_int64_t key_size;
> > >grub_int64_t priority;
> > >struct
> > > @@ -103,6 +104,7 @@ typedef struct grub_luks2_keyslot
> > > grub_luks2_keyslot_t;
> > >
> > >  struct grub_luks2_segment
> > >  {
> > > +  grub_uint64_t slot_key;
> >
> > Ditto. The code below uses keyslot instead...
> >
> > >grub_uint64_t offset;
> > >const char *size;
> > >const char *encryption;
> > > @@ -112,6 +114,7 @@ typedef struct grub_luks2_segment
> > > grub_luks2_segment_t;
> > >
> > >  struct grub_luks2_digest
> > >  {
> > > +  grub_uint64_t slot_key;
> > >/* Both keyslots and segments are interpreted as bitfields here
> > > */ grub_uint64_t  keyslots;
> > >grub_uint64_t  segments;
> > > @@ -259,12 +262,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->slot_key, &keyslot, NULL) ||

Should not keyslot be renamed to json_slot_key too?

Daniel

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

Re: [PATCH v6 02/12] luks2: Use more intuitive slot key instead of index in user messages.

2020-12-03 Thread Daniel Kiper 
On Thu, Dec 03, 2020 at 01:24:18AM -0600, Glenn Washburn wrote:
> On Wed, 2 Dec 2020 18:23:08 +0100
> Daniel Kiper  wrote:
>
> > On Fri, Nov 27, 2020 at 03:03:34AM -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. Error messages now distinguish
> > > between indexes and slot keys. The former include the string
> > > "index" in the error string, and the later are surrounded in quotes.
> >
> > This should be split into two separate patches. One should add "index"
> > to the messages another should add quotes. Well, I am not convinced
> > that quotes are good differentiators... I would drop them or rephrase
> > messages to really differentiate indexes and slot keys.
>
> I thought the quotes would make more intuitive sense to a user because
> the value would also be quoted in the json which would be output when
> using cryptosetup with --debug-json.  I can split it in to two patches,
> but just tell me how you want me to rephrase and I will.  No need for
> another round of reviews.

OK, let's stick with the quotes then.

Daniel

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

Re: [PATCH v6 04/12] cryptodisk: Replace some literals with constants in grub_cryptodisk_endecrypt

2020-12-03 Thread Daniel Kiper 
On Thu, Dec 03, 2020 at 02:29:11AM -0600, Glenn Washburn wrote:
> On Wed, 2 Dec 2020 18:37:42 +0100
> Daniel Kiper  wrote:
>
> > On Fri, Nov 27, 2020 at 03:03:36AM -0600, Glenn Washburn wrote:
> > > This should improve readability of code by providing clues as to
> > > what the value represents. The new macro GRUB_TYPE_BITS(type)
> > > returns the number of bits allocated for type. Also add
> > > GRUB_TYPE_U_MAX/MIN(type) macros to get the max/min values for an
> > > unsigned number with size of type.
> >
> > Two separate patches please...
>
> Ok.
>
> > > Signed-off-by: Glenn Washburn 
> > > ---
> > >  grub-core/disk/cryptodisk.c | 13 +++--
> > >  include/grub/types.h|  5 +
> > >  2 files changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c
> > > b/grub-core/disk/cryptodisk.c index 473c93976..31b73c535 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -284,22 +284,23 @@ grub_cryptodisk_endecrypt (struct
> > > grub_cryptodisk *dev, iv[1] = grub_cpu_to_le32 (sector >> 32);
> > > /* 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 >> (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);
> > > + & 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 >> 32);
> > > - iv[sz - 1] = grub_cpu_to_be32 (num & 0x);
> > > + iv[sz - 2] = grub_cpu_to_be32 (num >> GRUB_TYPE_BITS
> > > (iv[0]));
> > > + 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 f22055f98..29d807f71 100644
> > > --- a/include/grub/types.h
> > > +++ b/include/grub/types.h
> > > @@ -72,6 +72,8 @@
> > >  # endif
> > >  #endif
> > >
> > > +#define GRUB_TYPE_BITS(type) (sizeof(type) * GRUB_CHAR_BIT)
> >
> > This macro should go below "#ifndef __CHAR_BIT__ ...".
>
> Ack.
>
> > >  #ifndef __CHAR_BIT__
> > >  # error __CHAR_BIT__ is not defined
> > >  #elif __CHAR_BIT__ != 8
> > > @@ -159,6 +161,9 @@ typedef grub_int32_t  grub_ssize_t;
> > >  #endif
> > >  # define GRUB_LONG_MIN (-GRUB_LONG_MAX - 1)
> > >
> > > +#define GRUB_TYPE_U_MAX(type) ((typeof (1ULL))((typeof
> > > (type))(~0)))
> >
> > (typeof (1ULL)) == (unsigned long long)?
>
> I'll change it.
>
> > > +#define GRUB_TYPE_U_MIN(type) 0ULL
> >
> > I am not sure why you cast everything to ULL/unsigned long long.
> > Should not the final cast be to (typeof (type))?
>
> I think that to as great as an extent as possible macros should behave
> like functions.  So in this case, its "return type" should not change
> depending on the input arguments.  This can be confusing and the source
> of errors.  Sure, you can say that the user of the macro can just do
> the cast herself, but why add potentially unnecessary steps.
>
> Also, I wanted to unsure that the result was an unsigned type,
> regardless of signedness of input type. And using the largest unsigned
> type seemed the best because I don't know how to take an arbitrary
> integer type can only convert its signedness.
>
> But at this point, I really don't care.  Tell me what you want it to be
> and I'll make it so.

OK, make sense for me. However, please put short comment before macros
why you are casting final result to unsigned long long.

Daniel

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

Re: [PATCH v6 05/12] luks2: grub_cryptodisk_t->total_sectors is the max number of device native sectors

2020-12-03 Thread Daniel Kiper 
On Thu, Dec 03, 2020 at 02:54:19AM -0600, Glenn Washburn wrote:
> On Wed, 2 Dec 2020 18:56:46 +0100
> Daniel Kiper  wrote:
>
> > On Fri, Nov 27, 2020 at 03:03:37AM -0600, Glenn Washburn wrote:
> > > 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. And
> > > counter-intuitively, grub_disk_get_size returns the total number of
> > > device native sectors.
> >
> > Should not we change the name to, e.g., grub_disk_native_sectors()
> > then? Could you do that?
>
> Sure, I can do that.

Thanks!

Daniel

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

Re: [PATCH v6 06/12] cryptodisk: Properly handle non-512 byte sized sectors

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:38AM -0600, Glenn Washburn wrote:
> 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 

Daniel

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

Re: [PATCH v6 07/12] luks2: Better error handling when setting up the cryptodisk

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:39AM -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 | 80 --
>  include/grub/disk.h| 16 +
>  2 files changed, 93 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index b7ed0642e..01f9608e5 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -597,12 +597,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 (i = 0; i < size; i++)
>  {
> +  typeof(source->total_sectors) max_crypt_sectors = 0;
> +
> +  grub_errno = GRUB_ERR_NONE;
>ret = luks2_get_keyslot (&keyslot, &digest, &segment, 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)
>   {
> @@ -616,11 +630,71 @@ 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.slot_key, 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;
> +   crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> 
> crypt->log_sector_size;

I think ">>" should not happen here...

> +   if (grub_errno == GRUB_ERR_NONE)
> + ;

It should happen here... Or to be exact...
  crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 << 
crypt->log_sector_size); // Am I right?
  crypt->total_sectors >>= crypt->log_sector_size;

> +   else if(grub_errno == GRUB_ERR_BAD_NUMBER)

Missing space before "("...

> + {
> +   /* TODO: Unparsable number-string, try to use the whole disk */
> +   grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
> +  " \"%s\" is not a parsable number\n",
> +  segment.slot_key, segment.size);

  crypt->total_sectors = max_crypt_sectors; ?

>

Re: [PATCH] arm-coreboot: Declare global struct ps2_state as static so symbols don't cause link failure

2020-12-03 Thread Glenn Washburn 
On Thu, 3 Dec 2020 00:15:27 -0600
Glenn Washburn  wrote:

> Here is a log file showing the build failure.
> 
> https://gitlab.com/grub2-testing/grub/-/jobs/885372725/raw

Accidentally deleted this pipeline. Here's another log with the same
error.

https://gitlab.com/grub2-testing/grub/-/jobs/888562636/raw

> On Thu,  3 Dec 2020 00:10:26 -0600
> Glenn Washburn  wrote:
> 
> > This appears to be a new issue surfaced by switching from gcc 9.3.0
> > to 10.1.0 cross compiling toolchain. When linking these two object
> > files together, there is an error generated: "multiple definition of
> > `ps2_state'". This only appears to be an issue when
> > --target=arm-linux-gnueabi and --with-platform=coreboot. There's no
> > need for these structs to have global as opposed to file scope, so
> > declaring them as static fixes this issue.
> > 
> > Signed-off-by: Glenn Washburn 
> > ---
> >  grub-core/term/arm/cros.c  | 2 +-
> >  grub-core/term/arm/pl050.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/grub-core/term/arm/cros.c b/grub-core/term/arm/cros.c
> > index 1ff9f8ccf..a17e49c32 100644
> > --- a/grub-core/term/arm/cros.c
> > +++ b/grub-core/term/arm/cros.c
> > @@ -30,7 +30,7 @@
> >  #include 
> >  #include 
> >  
> > -struct grub_ps2_state ps2_state;
> > +static struct grub_ps2_state ps2_state;
> >  
> >  struct grub_cros_ec_keyscan old_scan;
> >  
> > diff --git a/grub-core/term/arm/pl050.c b/grub-core/term/arm/pl050.c
> > index e4cda3056..b082243b0 100644
> > --- a/grub-core/term/arm/pl050.c
> > +++ b/grub-core/term/arm/pl050.c
> > @@ -29,7 +29,7 @@
> >  
> >  static volatile grub_uint32_t *pl050_regs;
> >  
> > -struct grub_ps2_state ps2_state;
> > +static struct grub_ps2_state ps2_state;
> >  
> >  static void
> >  keyboard_controller_wait_until_ready (void)

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

Re: [PATCH v6 08/12] luks2: Error check segment.sector_size

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:40AM -0600, Glenn Washburn wrote:
> 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 v6 09/12] whitespace: convert 8 spaces to tabs.

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:41AM -0600, Glenn Washburn wrote:
> 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 v6 10/12] mips: Enable __clzdi2()

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:42AM -0600, Glenn Washburn wrote:
> 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: Daniel Kiper 

This should be replaced with:

  Suggested-by: Daniel Kiper 
  Signed-off-by: Glenn Washburn 

In general if somebody suggests you something and you use they idea then
you should use Suggested-by instead of SOB.

Anyway, I can fix it before committing the patch if you are OK with me
adding your SOB.

Daniel

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

Re: [PATCH v6 11/12] misc: Add grub_log2ull macro for calculating log base 2 of 64-bit integers

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:43AM -0600, Glenn Washburn wrote:
> 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 v6 12/12] luks2: Use grub_log2ull to calculate log_sector_size and improve readability

2020-12-03 Thread Daniel Kiper 
On Fri, Nov 27, 2020 at 03:03:44AM -0600, Glenn Washburn wrote:
> 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

[PATCH 4/9] efi: Make shim_lock GUID and protocol type public

2020-12-03 Thread Javier Martinez Canillas 
From: Daniel Kiper 

The GUID will be used to properly detect and report UEFI Secure Boot
status to the x86 Linux kernel. The functionality will be added by
subsequent patches. The shim_lock protocol type is made public for
completeness.

Additionally, fix formatting of four preceding GUIDs.

Signed-off-by: Daniel Kiper 
Signed-off-by: Marco A Benatto 
Signed-off-by: Javier Martinez Canillas 
---

 grub-core/commands/efi/shim_lock.c | 12 
 include/grub/efi/api.h | 19 +++
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/grub-core/commands/efi/shim_lock.c 
b/grub-core/commands/efi/shim_lock.c
index 764098cfc83..d8f52d721c3 100644
--- a/grub-core/commands/efi/shim_lock.c
+++ b/grub-core/commands/efi/shim_lock.c
@@ -27,18 +27,6 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
-#define GRUB_EFI_SHIM_LOCK_GUID \
-  { 0x605dab50, 0xe046, 0x4300, \
-{ 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23 } \
-  }
-
-struct grub_efi_shim_lock_protocol
-{
-  grub_efi_status_t
-  (*verify) (void *buffer, grub_uint32_t size);
-};
-typedef struct grub_efi_shim_lock_protocol grub_efi_shim_lock_protocol_t;
-
 static grub_efi_guid_t shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
 static grub_efi_shim_lock_protocol_t *sl;
 
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 1dcaa12f59e..39733585b58 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -321,22 +321,27 @@
 
 #define GRUB_EFI_SAL_TABLE_GUID \
   { 0xeb9d2d32, 0x2d88, 0x11d3, \
-  { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
+{ 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
   }
 
 #define GRUB_EFI_HCDP_TABLE_GUID \
   { 0xf951938d, 0x620b, 0x42ef, \
-  { 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98 } \
+{ 0x82, 0x79, 0xa8, 0x4b, 0x79, 0x61, 0x78, 0x98 } \
   }
 
 #define GRUB_EFI_DEVICE_TREE_GUID \
   { 0xb1b621d5, 0xf19c, 0x41a5, \
-  { 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0 } \
+{ 0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0 } \
   }
 
 #define GRUB_EFI_VENDOR_APPLE_GUID \
   { 0x2B0585EB, 0xD8B8, 0x49A9,\
-  { 0x8B, 0x8C, 0xE2, 0x1B, 0x01, 0xAE, 0xF2, 0xB7 } \
+{ 0x8B, 0x8C, 0xE2, 0x1B, 0x01, 0xAE, 0xF2, 0xB7 } \
+  }
+
+#define GRUB_EFI_SHIM_LOCK_GUID \
+  { 0x605dab50, 0xe046, 0x4300, \
+{ 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23 } \
   }
 
 struct grub_efi_sal_system_table
@@ -1694,6 +1699,12 @@ struct grub_efi_block_io
 };
 typedef struct grub_efi_block_io grub_efi_block_io_t;
 
+struct grub_efi_shim_lock_protocol
+{
+  grub_efi_status_t (*verify) (void *buffer, grub_uint32_t size);
+};
+typedef struct grub_efi_shim_lock_protocol grub_efi_shim_lock_protocol_t;
+
 #if (GRUB_TARGET_SIZEOF_VOID_P == 4) || defined (__ia64__) \
   || defined (__aarch64__) || defined (__MINGW64__) || defined (__CYGWIN__) \
   || defined(__riscv)
-- 
2.28.0


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

[PATCH 7/9] efi: Add secure boot detection

2020-12-03 Thread Javier Martinez Canillas 
From: Daniel Kiper 

Introduce grub_efi_get_secureboot() function which returns whether
UEFI Secure Boot is enabled or not on UEFI systems.

Signed-off-by: Ignat Korchagin 
Signed-off-by: Daniel Kiper 
Signed-off-by: Marco A Benatto 
Signed-off-by: Javier Martinez Canillas 
---

 grub-core/Makefile.am   |   1 +
 grub-core/Makefile.core.def |   1 +
 grub-core/kern/efi/sb.c | 109 
 include/grub/efi/sb.h   |  40 +
 4 files changed, 151 insertions(+)
 create mode 100644 grub-core/kern/efi/sb.c
 create mode 100644 include/grub/efi/sb.h

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 3ea8e7ff45f..c6ba5b2d763 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -71,6 +71,7 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/command.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/device.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/disk.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/dl.h
+KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/efi/sb.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/env.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/env_private.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/err.h
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index b5f47fc41b5..68b9e9f68dc 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -203,6 +203,7 @@ kernel = {
   efi = term/efi/console.c;
   efi = kern/acpi.c;
   efi = kern/efi/acpi.c;
+  efi = kern/efi/sb.c;
   i386_coreboot = kern/i386/pc/acpi.c;
   i386_multiboot = kern/i386/pc/acpi.c;
   i386_coreboot = kern/acpi.c;
diff --git a/grub-core/kern/efi/sb.c b/grub-core/kern/efi/sb.c
new file mode 100644
index 000..19658d9626d
--- /dev/null
+++ b/grub-core/kern/efi/sb.c
@@ -0,0 +1,109 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2020  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see .
+ *
+ *  UEFI Secure Boot related checkings.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Determine whether we're in secure boot mode.
+ *
+ * Please keep the logic in sync with the Linux kernel,
+ * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
+ */
+grub_uint8_t
+grub_efi_get_secureboot (void)
+{
+  static grub_efi_guid_t efi_variable_guid = GRUB_EFI_GLOBAL_VARIABLE_GUID;
+  static grub_efi_guid_t efi_shim_lock_guid = GRUB_EFI_SHIM_LOCK_GUID;
+  grub_efi_status_t status;
+  grub_efi_uint32_t attr = 0;
+  grub_size_t size = 0;
+  grub_uint8_t *secboot = NULL;
+  grub_uint8_t *setupmode = NULL;
+  grub_uint8_t *moksbstate = NULL;
+  grub_uint8_t secureboot = GRUB_EFI_SECUREBOOT_MODE_UNKNOWN;
+  const char *secureboot_str = "UNKNOWN";
+
+  status = grub_efi_get_variable ("SecureBoot", &efi_variable_guid,
+ &size, (void **) &secboot);
+
+  if (status == GRUB_EFI_NOT_FOUND)
+{
+  secureboot = GRUB_EFI_SECUREBOOT_MODE_DISABLED;
+  goto out;
+}
+
+  if (status != GRUB_EFI_SUCCESS)
+goto out;
+
+  status = grub_efi_get_variable ("SetupMode", &efi_variable_guid,
+ &size, (void **) &setupmode);
+
+  if (status != GRUB_EFI_SUCCESS)
+goto out;
+
+  if ((*secboot == 0) || (*setupmode == 1))
+{
+  secureboot = GRUB_EFI_SECUREBOOT_MODE_DISABLED;
+  goto out;
+}
+
+  /*
+   * See if a user has put the shim into insecure mode. If so, and if the
+   * variable doesn't have the runtime attribute set, we might as well
+   * honor that.
+   */
+  status = grub_efi_get_variable_with_attributes ("MokSBState", 
&efi_shim_lock_guid,
+ &size, (void **) &moksbstate, 
&attr);
+
+  /* If it fails, we don't care why. Default to secure. */
+  if (status != GRUB_EFI_SUCCESS)
+{
+  secureboot = GRUB_EFI_SECUREBOOT_MODE_ENABLED;
+  goto out;
+}
+
+  if (!(attr & GRUB_EFI_VARIABLE_RUNTIME_ACCESS) && *moksbstate == 1)
+{
+  secureboot = GRUB_EFI_SECUREBOOT_MODE_DISABLED;
+  goto out;
+}
+
+  secureboot = GRUB_EFI_SECUREBOOT_MODE_ENABLED;
+
+ out:
+  grub_free (moksbstate);
+  grub_free (setupmode);
+  grub_free (secboot);
+
+  if (secureboot == GRUB_EFI_SECUREBOOT_MODE_DISABLED)
+secureboot_str = "Disabled";
+  else if (secureboot == GRUB_EFI_SECUREBO

[PATCH 1/9] i386: Don't include in coreboot and ieee1275 startup.S

2020-12-03 Thread Javier Martinez Canillas 
Nothing defined in the header file is used in the assembly code but it
may lead to build errors if some headers are included through this and
contains definitions that are not recognized by the assembler, e.g.:

../include/grub/types.h: Assembler messages:
../include/grub/types.h:76: Error: no such instruction: `typedef signed char 
grub_int8_t'
../include/grub/types.h:77: Error: no such instruction: `typedef short 
grub_int16_t'
../include/grub/types.h:78: Error: no such instruction: `typedef int 
grub_int32_t'

Signed-off-by: Javier Martinez Canillas 
---

 grub-core/kern/i386/coreboot/startup.S | 1 -
 grub-core/kern/i386/ieee1275/startup.S | 1 -
 2 files changed, 2 deletions(-)

diff --git a/grub-core/kern/i386/coreboot/startup.S 
b/grub-core/kern/i386/coreboot/startup.S
index c8486548de0..df6adbabb5b 100644
--- a/grub-core/kern/i386/coreboot/startup.S
+++ b/grub-core/kern/i386/coreboot/startup.S
@@ -18,7 +18,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
diff --git a/grub-core/kern/i386/ieee1275/startup.S 
b/grub-core/kern/i386/ieee1275/startup.S
index 245583bdb71..62cf348e014 100644
--- a/grub-core/kern/i386/ieee1275/startup.S
+++ b/grub-core/kern/i386/ieee1275/startup.S
@@ -18,7 +18,6 @@
 
 #include 
 #include 
-#include 
 #include 
 #include 
 
-- 
2.28.0


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

[PATCH 2/9] include/grub/i386/linux.h: Include missing header

2020-12-03 Thread Javier Martinez Canillas 
This header uses types defined in  but does not include it,
which leads to compile errors like the following:

In file included from ../include/grub/cpu/linux.h:19,
 from kern/efi/sb.c:21:
../include/grub/i386/linux.h:80:3: error: unknown type name ‘grub_uint64_t’
   80 |   grub_uint64_t addr;

Signed-off-by: Javier Martinez Canillas 
---

 include/grub/i386/linux.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
index ce30e7fb01b..6da5f030fd1 100644
--- a/include/grub/i386/linux.h
+++ b/include/grub/i386/linux.h
@@ -19,6 +19,8 @@
 #ifndef GRUB_I386_LINUX_HEADER
 #define GRUB_I386_LINUX_HEADER 1
 
+#include 
+
 #define GRUB_LINUX_I386_MAGIC_SIGNATURE0x53726448  /* "HdrS" */
 #define GRUB_LINUX_DEFAULT_SETUP_SECTS 4
 #define GRUB_LINUX_INITRD_MAX_ADDRESS  0x37FF
-- 
2.28.0


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

[PATCH 3/9] arm/term: Fix linking error due multiple ps2_state definitions

2020-12-03 Thread Javier Martinez Canillas 
When building with --target=arm-linux-gnu --with-platform=coreboot
a linking error occurs caused by multiple definitions of the
ps2_state variable.

Mark them as static since they aren't used outside their compilation unit.

Signed-off-by: Javier Martinez Canillas 
---

 grub-core/term/arm/cros.c  | 2 +-
 grub-core/term/arm/pl050.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/term/arm/cros.c b/grub-core/term/arm/cros.c
index 1ff9f8ccfb8..a17e49c325c 100644
--- a/grub-core/term/arm/cros.c
+++ b/grub-core/term/arm/cros.c
@@ -30,7 +30,7 @@
 #include 
 #include 
 
-struct grub_ps2_state ps2_state;
+static struct grub_ps2_state ps2_state;
 
 struct grub_cros_ec_keyscan old_scan;
 
diff --git a/grub-core/term/arm/pl050.c b/grub-core/term/arm/pl050.c
index e4cda305666..b082243b032 100644
--- a/grub-core/term/arm/pl050.c
+++ b/grub-core/term/arm/pl050.c
@@ -29,7 +29,7 @@
 
 static volatile grub_uint32_t *pl050_regs;
 
-struct grub_ps2_state ps2_state;
+static struct grub_ps2_state ps2_state;
 
 static void
 keyboard_controller_wait_until_ready (void)
-- 
2.28.0


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

[PATCH 0/9] Add UEFI Secure Boot detection and report the status to Linux

2020-12-03 Thread Javier Martinez Canillas 

This patch series adds support for the GRUB to detect the UEFI Secure Boot
status using the SecureBoot and SetupMode EFI variables. It also reports
this to Linux by setting the .secure_boot field of struct boot_params.

Besides that, it contains some cleanups and fixes mostly around EFI support.

Best regards,
Javier


Daniel Kiper (5):
  efi: Make shim_lock GUID and protocol type public
  efi: Return grub_efi_status_t from grub_efi_get_variable()
  efi: Add a function to read EFI variables with attributes
  efi: Add secure boot detection
  loader/linux: Report the UEFI Secure Boot status to the Linux kernel

Javier Martinez Canillas (4):
  i386: Don't include  in coreboot and ieee1275
startup.S
  include/grub/i386/linux.h: Include missing  header
  arm/term: Fix linking error due multiple ps2_state definitions
  efi: Only register shim_lock verifier if shim_lock protocol is found
and SB enabled

 grub-core/Makefile.am  |   1 +
 grub-core/Makefile.core.def|   1 +
 grub-core/commands/efi/efifwsetup.c|   8 +-
 grub-core/commands/efi/shim_lock.c |  28 ++-
 grub-core/kern/efi/efi.c   |  30 +--
 grub-core/kern/efi/sb.c| 109 +
 grub-core/kern/i386/coreboot/startup.S |   1 -
 grub-core/kern/i386/ieee1275/startup.S |   1 -
 grub-core/loader/i386/linux.c  |   6 +-
 grub-core/term/arm/cros.c  |   2 +-
 grub-core/term/arm/pl050.c |   2 +-
 grub-core/video/efi_gop.c  |   2 +-
 include/grub/efi/api.h |  19 -
 include/grub/efi/efi.h |  12 ++-
 include/grub/efi/sb.h  |  40 +
 include/grub/i386/linux.h  |  10 ++-
 16 files changed, 225 insertions(+), 47 deletions(-)
 create mode 100644 grub-core/kern/efi/sb.c
 create mode 100644 include/grub/efi/sb.h

-- 
2.28.0


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

[PATCH 5/9] efi: Return grub_efi_status_t from grub_efi_get_variable()

2020-12-03 Thread Javier Martinez Canillas 
From: Daniel Kiper 

This is needed to properly detect and report UEFI Secure Boot status
to the x86 Linux kernel. The functionality will be added by subsequent
patches.

Signed-off-by: Daniel Kiper 
Signed-off-by: Marco A Benatto 
Signed-off-by: Javier Martinez Canillas 
---

 grub-core/commands/efi/efifwsetup.c |  8 
 grub-core/kern/efi/efi.c| 16 +---
 grub-core/video/efi_gop.c   |  2 +-
 include/grub/efi/efi.h  |  7 ---
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/grub-core/commands/efi/efifwsetup.c 
b/grub-core/commands/efi/efifwsetup.c
index 7a137a72a2f..eaca0328388 100644
--- a/grub-core/commands/efi/efifwsetup.c
+++ b/grub-core/commands/efi/efifwsetup.c
@@ -38,8 +38,8 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
   grub_size_t oi_size;
   grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
-  old_os_indications = grub_efi_get_variable ("OsIndications", &global,
- &oi_size);
+  grub_efi_get_variable ("OsIndications", &global, &oi_size,
+(void **) &old_os_indications);
 
   if (old_os_indications != NULL && oi_size == sizeof (os_indications))
 os_indications |= *old_os_indications;
@@ -63,8 +63,8 @@ efifwsetup_is_supported (void)
   grub_size_t oi_size = 0;
   grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
 
-  os_indications_supported = grub_efi_get_variable ("OsIndicationsSupported",
-   &global, &oi_size);
+  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
+(void **) &os_indications_supported);
 
   if (!os_indications_supported)
 return 0;
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index e0165e74c58..9403b12cd78 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -223,9 +223,9 @@ grub_efi_set_variable(const char *var, const 
grub_efi_guid_t *guid,
   return grub_error (GRUB_ERR_IO, "could not set EFI variable `%s'", var);
 }
 
-void *
+grub_efi_status_t
 grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
-  grub_size_t *datasize_out)
+  grub_size_t *datasize_out, void **data_out)
 {
   grub_efi_status_t status;
   grub_efi_uintn_t datasize = 0;
@@ -234,13 +234,14 @@ grub_efi_get_variable (const char *var, const 
grub_efi_guid_t *guid,
   void *data;
   grub_size_t len, len16;
 
+  *data_out = NULL;
   *datasize_out = 0;
 
   len = grub_strlen (var);
   len16 = len * GRUB_MAX_UTF16_PER_UTF8;
   var16 = grub_calloc (len16 + 1, sizeof (var16[0]));
   if (!var16)
-return NULL;
+return GRUB_EFI_OUT_OF_RESOURCES;
   len16 = grub_utf8_to_utf16 (var16, len16, (grub_uint8_t *) var, len, NULL);
   var16[len16] = 0;
 
@@ -251,14 +252,14 @@ grub_efi_get_variable (const char *var, const 
grub_efi_guid_t *guid,
   if (status != GRUB_EFI_BUFFER_TOO_SMALL || !datasize)
 {
   grub_free (var16);
-  return NULL;
+  return status;
 }
 
   data = grub_malloc (datasize);
   if (!data)
 {
   grub_free (var16);
-  return NULL;
+  return GRUB_EFI_OUT_OF_RESOURCES;
 }
 
   status = efi_call_5 (r->get_variable, var16, guid, NULL, &datasize, data);
@@ -266,12 +267,13 @@ grub_efi_get_variable (const char *var, const 
grub_efi_guid_t *guid,
 
   if (status == GRUB_EFI_SUCCESS)
 {
+  *data_out = data;
   *datasize_out = datasize;
-  return data;
+  return status;
 }
 
   grub_free (data);
-  return NULL;
+  return status;
 }
 
 #pragma GCC diagnostic ignored "-Wcast-align"
diff --git a/grub-core/video/efi_gop.c b/grub-core/video/efi_gop.c
index be446f8d291..7fe0cdabf50 100644
--- a/grub-core/video/efi_gop.c
+++ b/grub-core/video/efi_gop.c
@@ -316,7 +316,7 @@ grub_video_gop_get_edid (struct grub_video_edid_info 
*edid_info)
   char edidname[] = "agp-internal-edid";
   grub_size_t datasize;
   grub_uint8_t *data;
-  data = grub_efi_get_variable (edidname, &efi_var_guid, &datasize);
+  grub_efi_get_variable (edidname, &efi_var_guid, &datasize, (void **) 
&data);
   if (data && datasize > 16)
{
  copy_size = datasize - 16;
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e90e00dc431..8b2a0f1f590 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -74,9 +74,10 @@ grub_err_t EXPORT_FUNC (grub_efi_set_virtual_address_map) 
(grub_efi_uintn_t memo
   grub_efi_uintn_t 
descriptor_size,
   grub_efi_uint32_t 
descriptor_version,
   
grub_efi_memory_descriptor_t *virtual_map);
-void *EXPORT_FUNC (grub_efi_get_variable) (const char *variable,
-  const grub_efi_guid_t *guid,
-  grub

[PATCH 6/9] efi: Add a function to read EFI variables with attributes

2020-12-03 Thread Javier Martinez Canillas 
From: Daniel Kiper 

It will be used to properly detect and report UEFI Secure Boot status to
the x86 Linux kernel. The functionality will be added by subsequent patches.

Signed-off-by: Ignat Korchagin 
Signed-off-by: Daniel Kiper 
Signed-off-by: Marco A Benatto 
Signed-off-by: Javier Martinez Canillas 
---

 grub-core/kern/efi/efi.c | 16 +---
 include/grub/efi/efi.h   |  5 +
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 9403b12cd78..2942b8e355d 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -224,8 +224,11 @@ grub_efi_set_variable(const char *var, const 
grub_efi_guid_t *guid,
 }
 
 grub_efi_status_t
-grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
-  grub_size_t *datasize_out, void **data_out)
+grub_efi_get_variable_with_attributes (const char *var,
+  const grub_efi_guid_t *guid,
+  grub_size_t *datasize_out,
+  void **data_out,
+  grub_efi_uint32_t *attributes)
 {
   grub_efi_status_t status;
   grub_efi_uintn_t datasize = 0;
@@ -262,7 +265,7 @@ grub_efi_get_variable (const char *var, const 
grub_efi_guid_t *guid,
   return GRUB_EFI_OUT_OF_RESOURCES;
 }
 
-  status = efi_call_5 (r->get_variable, var16, guid, NULL, &datasize, data);
+  status = efi_call_5 (r->get_variable, var16, guid, attributes, &datasize, 
data);
   grub_free (var16);
 
   if (status == GRUB_EFI_SUCCESS)
@@ -276,6 +279,13 @@ grub_efi_get_variable (const char *var, const 
grub_efi_guid_t *guid,
   return status;
 }
 
+grub_efi_status_t
+grub_efi_get_variable (const char *var, const grub_efi_guid_t *guid,
+  grub_size_t *datasize_out, void **data_out)
+{
+  return grub_efi_get_variable_with_attributes (var, guid, datasize_out, 
data_out, NULL);
+}
+
 #pragma GCC diagnostic ignored "-Wcast-align"
 
 /* Search the mods section from the PE32/PE32+ image. This code uses
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 8b2a0f1f590..83d958f9945 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -74,6 +74,11 @@ grub_err_t EXPORT_FUNC (grub_efi_set_virtual_address_map) 
(grub_efi_uintn_t memo
   grub_efi_uintn_t 
descriptor_size,
   grub_efi_uint32_t 
descriptor_version,
   
grub_efi_memory_descriptor_t *virtual_map);
+grub_efi_status_t EXPORT_FUNC (grub_efi_get_variable_with_attributes) (const 
char *variable,
+  const 
grub_efi_guid_t *guid,
+  
grub_size_t *datasize_out,
+  void 
**data_out,
+  
grub_efi_uint32_t *attributes);
 grub_efi_status_t EXPORT_FUNC (grub_efi_get_variable) (const char *variable,
   const grub_efi_guid_t 
*guid,
   grub_size_t 
*datasize_out,
-- 
2.28.0


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

[PATCH 9/9] loader/linux: Report the UEFI Secure Boot status to the Linux kernel

2020-12-03 Thread Javier Martinez Canillas 
From: Daniel Kiper 

Now that the GRUB has a grub_efi_get_secureboot() function to check the
UEFI Secure Boot status, use it to report that to the Linux kernel.

Signed-off-by: Ignat Korchagin 
Signed-off-by: Daniel Kiper 
Signed-off-by: Marco A Benatto 
Signed-off-by: Javier Martinez Canillas 
---

 grub-core/loader/i386/linux.c | 6 +-
 include/grub/i386/linux.h | 8 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 976af3fae87..d7e68658f43 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -46,6 +46,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 #ifdef GRUB_MACHINE_EFI
 #include 
+#include 
 #define HAS_VGA_TEXT 0
 #define DEFAULT_VIDEO_MODE "auto"
 #define ACCEPTS_PURE_TEXT 0
@@ -583,6 +584,9 @@ grub_linux_boot (void)
 grub_efi_uintn_t efi_desc_size;
 grub_size_t efi_mmap_target;
 grub_efi_uint32_t efi_desc_version;
+
+ctx.params->secure_boot = grub_efi_get_secureboot ();
+
 err = grub_efi_finish_boot_services (&efi_mmap_size, efi_mmap_buf, NULL,
 &efi_desc_size, &efi_desc_version);
 if (err)
@@ -794,7 +798,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 
   linux_params.code32_start = prot_mode_target + lh.code32_start - 
GRUB_LINUX_BZIMAGE_ADDR;
   linux_params.kernel_alignment = (1 << align);
-  linux_params.ps_mouse = linux_params.padding10 = 0;
+  linux_params.ps_mouse = linux_params.padding11 = 0;
   linux_params.type_of_loader = GRUB_LINUX_BOOT_LOADER_TYPE;
 
   /* These two are used (instead of cmd_line_ptr) by older versions of Linux,
diff --git a/include/grub/i386/linux.h b/include/grub/i386/linux.h
index 6da5f030fd1..eddf9251d9a 100644
--- a/include/grub/i386/linux.h
+++ b/include/grub/i386/linux.h
@@ -277,7 +277,11 @@ struct linux_kernel_params
 
   grub_uint8_t mmap_size;  /* 1e8 */
 
-  grub_uint8_t padding9[0x1f1 - 0x1e9];
+  grub_uint8_t padding9[0x1ec - 0x1e9];
+
+  grub_uint8_t secure_boot; /* 1ec */
+
+  grub_uint8_t padding10[0x1f1 - 0x1ed];
 
   /* Linux setup header copy - BEGIN. */
   grub_uint8_t setup_sects;/* The size of the setup in sectors */
@@ -288,7 +292,7 @@ struct linux_kernel_params
   grub_uint16_t vid_mode;  /* Video mode control */
   grub_uint16_t root_dev;  /* Default root device number */
 
-  grub_uint8_t padding10;  /* 1fe */
+  grub_uint8_t padding11;  /* 1fe */
   grub_uint8_t ps_mouse;   /* 1ff */
 
   grub_uint16_t jump;  /* Jump instruction */
-- 
2.28.0


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

[PATCH 8/9] efi: Only register shim_lock verifier if shim_lock protocol is found and SB enabled

2020-12-03 Thread Javier Martinez Canillas 
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 
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 
(&shim_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 (&shim_lock_guid, 0);
-  grub_verifier_register (&shim_lock);
+  grub_efi_shim_lock_protocol_t *sl = grub_efi_locate_protocol 
(&shim_lock_guid, 0);
 
-  if (!sl)
+  if (sl == NULL || grub_efi_get_secureboot () != 
GRUB_EFI_SECUREBOOT_MODE_ENABLED)
 return;
 
+  grub_verifier_register (&shim_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

Re: [PATCH v6 10/12] mips: Enable __clzdi2()

2020-12-03 Thread Glenn Washburn 
On Thu, 3 Dec 2020 15:00:14 +0100
Daniel Kiper  wrote:

> On Fri, Nov 27, 2020 at 03:03:42AM -0600, Glenn Washburn wrote:
> > 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: Daniel Kiper 
> 
> This should be replaced with:
> 
>   Suggested-by: Daniel Kiper 
>   Signed-off-by: Glenn Washburn 
> 
> In general if somebody suggests you something and you use they idea
> then you should use Suggested-by instead of SOB.
> 
> Anyway, I can fix it before committing the patch if you are OK with me
> adding your SOB.

Sure, I'm fine with you adding my SOB. I don't know if you're not
taking finished patches that come after unfinished patches, so I was
just going to fix and include in my next iteration.  If you're ready to
take these patches, then go ahead and I'll have less patches in the v7
series.

Glenn

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

Re: [PATCH v6 07/12] luks2: Better error handling when setting up the cryptodisk

2020-12-03 Thread Glenn Washburn 
On Thu, 3 Dec 2020 14:31:49 +0100
Daniel Kiper  wrote:

> On Fri, Nov 27, 2020 at 03:03:39AM -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 | 80
> > -- include/grub/disk.h|
> > 16 + 2 files changed, 93 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index b7ed0642e..01f9608e5 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -597,12 +597,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 (i = 0; i < size; i++)
> >  {
> > +  typeof(source->total_sectors) max_crypt_sectors = 0;
> > +
> > +  grub_errno = GRUB_ERR_NONE;
> >ret = luks2_get_keyslot (&keyslot, &digest, &segment, 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)
> > {
> > @@ -616,11 +630,71 @@ 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.slot_key,
> > 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;
> > + crypt->total_sectors = grub_strtoull (segment.size,
> > NULL, 10) >> crypt->log_sector_size;
> 
> I think ">>" should not happen here...

Do you think this because ">>" should not operate on a value returned
by a call to grub_strtoull that has errored?  I don't think there's any
problem because the return value is a number, so there's no harm in
using ">>" on a garbage number to get another garbage number, as long
as we don't use the value. (Its also not technically a garbage number,
just one of two values in the error case)

Also I didn't want to set total_sectors to grub_strtoull because it
could be a little confusing since the return value of grub_strtoull is
bytes not sectors.  And yet storing in tot

Re: [PATCH v6 01/12] luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest

2020-12-03 Thread Glenn Washburn 
On Thu, 3 Dec 2020 13:35:28 +0100
Daniel Kiper  wrote:

> On Thu, Dec 03, 2020 at 01:23:17AM -0600, Glenn Washburn wrote:
> > On Wed, 2 Dec 2020 18:01:47 +0100
> > Daniel Kiper  wrote:
> >
> > > On Fri, Nov 27, 2020 at 03:03:33AM -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
> > >
> > > I prefer if you add "()" to the function names, i.e.
> > > luks2_get_keyslot(), in the comments and commit messages. This
> > > way it is easier to understand what you mean.
> >
> > Ok.
> >
> > > > for both keyslot and segment slot keys.
> > > >
> > > > Signed-off-by: Glenn Washburn 
> > > > ---
> > > >  grub-core/disk/luks2.c | 13 -
> > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > index d96764a02..ab2c31dcd 100644
> > > > --- a/grub-core/disk/luks2.c
> > > > +++ b/grub-core/disk/luks2.c
> > > > @@ -65,6 +65,7 @@ typedef struct grub_luks2_header
> > > > grub_luks2_header_t;
> > > >
> > > >  struct grub_luks2_keyslot
> > > >  {
> > > > +  grub_uint64_t slot_key;
> > >
> > > Could you be more consistent and use keyslot instead of slot_key
> > > here?
> >
> > I intentionally chose not to use keyslot because I thought it was
> > confusing.  slot_key is not a keyslot in the sense of a slot for a
> > cryptographic key or key material as in the usage keyslot in the
> > struct named "grub_luks2_keyslot".  Its the key value of a "slot"
> > in a json associative array which is modeling a sparse array.
> > Perhaps just "key" might be more to your liking?
> 
> IMO "key" is more confusing in this context and it would require at
> least a comment to clarify what it means. Is not json_slot_key better
> then? Probably with some comment what is it too...

Yeah, that's a better name in terms of its descriptive power, but feels
kinda long.  I'm fine with using that though, and will use that for the
next patch series, unless I hear otherwise.

> > > >grub_int64_t key_size;
> > > >grub_int64_t priority;
> > > >struct
> > > > @@ -103,6 +104,7 @@ typedef struct grub_luks2_keyslot
> > > > grub_luks2_keyslot_t;
> > > >
> > > >  struct grub_luks2_segment
> > > >  {
> > > > +  grub_uint64_t slot_key;
> > >
> > > Ditto. The code below uses keyslot instead...
> > >
> > > >grub_uint64_t offset;
> > > >const char   *size;
> > > >const char   *encryption;
> > > > @@ -112,6 +114,7 @@ typedef struct grub_luks2_segment
> > > > grub_luks2_segment_t;
> > > >
> > > >  struct grub_luks2_digest
> > > >  {
> > > > +  grub_uint64_t slot_key;
> > > >/* Both keyslots and segments are interpreted as bitfields
> > > > here */ grub_uint64_t   keyslots;
> > > >grub_uint64_tsegments;
> > > > @@ -259,12 +262,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->slot_key, &keyslot, NULL) ||
> 
> Should not keyslot be renamed to json_slot_key too?

If I understand you correctly, you're asking about the variable keyslot
of type grub_json_t.  Here keyslot is a json object representing a
keyslot (ie collection of parameters used to decrypt an encrypted key).
It is not what I call a "slot key", which is the value of a key in a
json associative array.  So keyslot is a json object, where as a slot
key is always a json string (afaik).  I believe that it is currently
appropriately named.

Glenn

___
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

2020-12-03 Thread Andy Shevchenko 
On Sat, Nov 14, 2020 at 2:01 AM Daniel Kiper  wrote:

...

> The log specification should be as much as possible platform agnostic
> and self contained. The final version of this spec should be merged into
> existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone
> spec, e.g. as a part of OASIS Standards. The former seems better but is
> not perfect too...

With all respect... https://xkcd.com/927/


-- 
With Best Regards,
Andy Shevchenko

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

[PATCH v2] loopback: Do not automaticaly replace existing loopback dev, error instead

2020-12-03 Thread Glenn Washburn 
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 
---
 grub-core/disk/loopback.c | 18 +-
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/grub-core/disk/loopback.c b/grub-core/disk/loopback.c
index cdf9123fa..ef3a05fec 100644
--- a/grub-core/disk/loopback.c
+++ b/grub-core/disk/loopback.c
@@ -92,24 +92,16 @@ grub_cmd_loopback (grub_extcmd_context_t ctxt, int argc, 
char **args)
   if (argc < 2)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
+  /* Check that a device with requested name does not already exist.  */
+  for (newdev = loopback_list; newdev; newdev = newdev->next)
+if (grub_strcmp (newdev->devname, args[0]) == 0)
+  return grub_error(GRUB_ERR_BAD_ARGUMENT, "device name already exists");
+
   file = grub_file_open (args[1], GRUB_FILE_TYPE_LOOPBACK
 | GRUB_FILE_TYPE_NO_DECOMPRESS);
   if (! file)
 return grub_errno;
 
-  /* First try to replace the old device.  */
-  for (newdev = loopback_list; newdev; newdev = newdev->next)
-if (grub_strcmp (newdev->devname, args[0]) == 0)
-  break;
-
-  if (newdev)
-{
-  grub_file_close (newdev->file);
-  newdev->file = file;
-
-  return 0;
-}
-
   /* Unable to replace it, make a new entry.  */
   newdev = grub_malloc (sizeof (struct grub_loopback));
   if (! newdev)
-- 
2.27.0


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