On Thu, 3 Dec 2020 14:31:49 +0100
Daniel Kiper <dki...@net-space.pl> 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 <developm...@efficientek.com>
> > ---
> > 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 total_sectors. I could use a
local variable, but was wanting to avoid that. But I'm fine with this
suggestion.
> > + 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;
I don't think we need to decrypt partial blocks at the end.
Cryptsetup/dm-crypt enforces disk sizes that are multiples of the
sector size. So if the size is not a multiple of blocksize, there's a
bug somewhere in cryptsetup/dm-crypt, and we don't need to deal with
the partial block. At worse the use loses access to a partial block at
the end that they should never have had access to in the first place.
> > + else if(grub_errno == GRUB_ERR_BAD_NUMBER)
>
> Missing space before "("...
Ack.
> > + {
> > + /* 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; ?
Sure, I didn't think you'd want this since it potentially entails
trying to read past the end of the luks data, and thus getting
random data, which might crash (buggy) filesystem read code.
> > + continue;
>
> ...and then drop 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_UINT64_T"\" size"
> > + " %s overflowed 64-bit
> > unsigned integer,"
> > + " the end of the crypto
> > device will be"
> > + " inaccessible\n",
> > + segment.slot_key,
> > segment.size);
> > + if (crypt->total_sectors > max_crypt_sectors)
> > + crypt->total_sectors = max_crypt_sectors;
> > + }
> > + }
> > +
> > + if (crypt->total_sectors == 0)
> > + {
> > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has zero"
> > + " sectors, skipping\n",
> > + segment.slot_key);
> > + continue;
> > + }
> > + else if (max_crypt_sectors < (crypt->offset_sectors +
> > crypt->total_sectors))
> > + {
> > + grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has last"
> > + " data position greater than
> > source disk size,"
> > + " the end of the crypto device
> > will be"
> > + " inaccessible\n",
> > + segment.slot_key);
> > + /* Allow decryption up to the end of the source disk. */
> > + crypt->total_sectors = max_crypt_sectors -
> > crypt->offset_sectors;
> > + }
> >
> > ret = luks2_decrypt_key (candidate_key, source, crypt,
> > &keyslot, (const grub_uint8_t *) passphrase, grub_strlen
> > (passphrase)); diff --git a/include/grub/disk.h
> > b/include/grub/disk.h index 132a1bb75..1a9e8fcf4 100644
> > --- a/include/grub/disk.h
> > +++ b/include/grub/disk.h
> > @@ -174,6 +174,22 @@ typedef struct grub_disk_memberlist
> > *grub_disk_memberlist_t; /* Return value of grub_disk_get_size() in
> > case disk size is unknown. */ #define GRUB_DISK_SIZE_UNKNOWN
> > 0xffffffffffffffffULL
> >
> > +/* Convert sector number from disk sized sectors to a log-size
> > sized sector. */ +static inline grub_disk_addr_t
> > +grub_disk_convert_sector (grub_disk_t disk,
> > + grub_disk_addr_t sector,
> > + grub_size_t log_sector_size)
> > +{
> > + if (disk->log_sector_size < log_sector_size)
> > + {
> > + /* Round up to the nearest log_sector_size sized sector. */
> > + sector += 1ULL << ((log_sector_size / disk->log_sector_size)
> > - 1);
>
> ALIGN_UP()?
Sure, I can do that and then remove the comment.
> > + return sector >> (log_sector_size - disk->log_sector_size);
> > + }
> > + else
> > + return sector << (disk->log_sector_size - log_sector_size);
> > +}
> > +
> > /* Convert to GRUB native disk sized sector from disk sized
> > sector. */ static inline grub_disk_addr_t
> > grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t
> > sector)
>
> Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
