On Mon, 29 Aug 2022 07:38:24 +0200 Patrick Steinhardt <p...@pks.im> wrote:
> On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote: > > A user can now specify UUID strings with dashes, instead of having to remove > > dashes. This is backwards-compatability preserving and also fixes a source > > of user confusion over the inconsistency with how UUIDs are specified > > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the > > reference implementation for LUKS, displays and generates UUIDs with dashes > > there has been additional confusion when using the UUID strings from > > cryptsetup as exact input into GRUB does not find the expected cryptodisk. > > > > A new function grub_uuidcasecmp is added that is general enough to be used > > other places where UUIDs are being compared. > > > > Signed-off-by: Glenn Washburn <developm...@efficientek.com> > > --- > > grub-core/disk/cryptodisk.c | 4 ++-- > > grub-core/disk/geli.c | 2 +- > > grub-core/disk/luks.c | 21 ++++----------------- > > grub-core/disk/luks2.c | 15 ++++----------- > > include/grub/misc.h | 32 ++++++++++++++++++++++++++++++++ > > 5 files changed, 43 insertions(+), 31 deletions(-) > > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > > index e89430812..a4cd8445f 100644 > > --- a/grub-core/disk/cryptodisk.c > > +++ b/grub-core/disk/cryptodisk.c > > @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t > > disk) > > if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0) > > { > > for (dev = cryptodisk_list; dev != NULL; dev = dev->next) > > - if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0) > > + if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, > > sizeof (dev->uuid)) == 0) > > break; > > } > > else > > @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid) > > { > > grub_cryptodisk_t dev; > > for (dev = cryptodisk_list; dev != NULL; dev = dev->next) > > - if (grub_strcasecmp (dev->uuid, uuid) == 0) > > + if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0) > > return dev; > > return NULL; > > } > > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > > index e190066f9..722910d64 100644 > > --- a/grub-core/disk/geli.c > > +++ b/grub-core/disk/geli.c > > @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > return NULL; > > } > > > > - if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, > > uuid) != 0) > > + if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, > > uuid, sizeof (uuid)) != 0) > > { > > grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid); > > return NULL; > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > > index 7f837d52c..9e1e6a5d9 100644 > > --- a/grub-core/disk/luks.c > > +++ b/grub-core/disk/luks.c > > @@ -66,10 +66,7 @@ static grub_cryptodisk_t > > luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs) > > { > > grub_cryptodisk_t newdev; > > - const char *iptr; > > struct grub_luks_phdr header; > > - char *optr; > > - char uuid[sizeof (header.uuid) + 1]; > > char ciphername[sizeof (header.cipherName) + 1]; > > char ciphermode[sizeof (header.cipherMode) + 1]; > > char hashspec[sizeof (header.hashSpec) + 1]; > > @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > || grub_be_to_cpu16 (header.version) != 1) > > return NULL; > > > > - grub_memset (uuid, 0, sizeof (uuid)); > > - optr = uuid; > > - for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)]; > > - iptr++) > > + if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, > > header.uuid, sizeof (header.uuid)) != 0) > > { > > - if (*iptr != '-') > > - *optr++ = *iptr; > > - } > > - *optr = 0; > > - > > - if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, > > uuid) != 0) > > - { > > - grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid); > > + grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid); > > return NULL; > > } > > I haven't noticed this in my previous review round, but I think this is > wrong because `header.uuid` is never NUL-terminated. It is explicitly 40 > characters long and read directly from disk, so there wouldn't be any > room for it to be NUL-terminated. Why not? UUIDs are 32 hex characters with typically 4 dashes, that makes 37 bytes needed including the NULL byte. In fact, I believe that cryptsetup always creates the uuid field NULL terminated. Also, the LUKS1 spec, and by extension LUKS2 spec, mandates NULL termination. The uuid field is specified as "char[]" and "char[], a string stored as null terminated sequence of 8-bit characters7". > > So we'd rather have to: > > grub_dprintf ("luks2", "%.*s != %s\n", header.uuid, sizeof (header.uuid), > cargs->search_uuid); > > Sorry for not catching this in my first round. This seems reasonable for what should be a very uncommon and non-spec compliant case of having a uuid field with no NULL byte. I originally had a patch before this that explicitly put a NULL byte at the end of header.uuid so this problem didn't exist. I dropped it because it was generally redundant as pointed out by Daniel, but I didn't take this case into account. Perhaps its worth merging that with this one for clarity. > > > > > @@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > newdev->source_disk = NULL; > > newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE; > > newdev->total_sectors = grub_disk_native_sectors (disk) - > > newdev->offset_sectors; > > - grub_memcpy (newdev->uuid, uuid, sizeof (uuid)); > > + grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid)); > > newdev->modname = "luks"; > > This should be fine though because `newdev->uuid` is initialized to > all-zeroes. > > > /* Configure the hash used for the AF splitter and HMAC. */ > > @@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > return NULL; > > } > > > > - COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid)); > > + COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid)); > > return newdev; > > } > > This has an off-by-one bug now though: `sizeof(uuid)` is not the same as > `sizeof(header.uuid)`, but instead it was `sizeof(header.uuid)+1` to > account for the trailing NUL-byte. So we have to make this: > > COMPILE_TIME_ASSERT (sizeof (newdev->uuid) > sizeof (header.uuid)); I don't think this is needed per above. Please let me know if I've missed something and thanks for being cautious. Glenn > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c > > index 5b3b36c8a..1174c4c2b 100644 > > --- a/grub-core/disk/luks2.c > > +++ b/grub-core/disk/luks2.c > > @@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > { > > grub_cryptodisk_t cryptodisk; > > grub_luks2_header_t header; > > - char uuid[sizeof (header.uuid) + 1]; > > - grub_size_t i, j; > > > > if (cargs->check_boot) > > return NULL; > > @@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > return NULL; > > } > > > > - for (i = 0, j = 0; i < sizeof (header.uuid); i++) > > - if (header.uuid[i] != '-') > > - uuid[j++] = header.uuid[i]; > > - uuid[j] = '\0'; > > - > > - if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, > > uuid) != 0) > > + if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, > > header.uuid, sizeof (header.uuid)) != 0) > > { > > - grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid); > > + grub_dprintf ("luks2", "%s != %s\n", header.uuid, > > cargs->search_uuid); > > return NULL; > > } > > Same here. > > > @@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t > > cargs) > > if (!cryptodisk) > > return NULL; > > > > - COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid)); > > - grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid)); > > + COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid)); > > + grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid)); > > And here. > > Patrick > > > cryptodisk->modname = "luks2"; > > return cryptodisk; > > diff --git a/include/grub/misc.h b/include/grub/misc.h > > index 1c25c1767..76c5c7992 100644 > > --- a/include/grub/misc.h > > +++ b/include/grub/misc.h > > @@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, > > grub_size_t n) > > - (int) grub_tolower ((grub_uint8_t) *s2); > > } > > > > +/* > > + * Do a case insensitive compare of two UUID strings by ignoring all > > dashes. > > + * Note that the parameter n, is the number of significant characters to > > + * compare, where significant characters are any except the dash. > > + */ > > +static inline int > > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n) > > +{ > > + if (n == 0) > > + return 0; > > + > > + while (*uuid1 && *uuid2 && --n) > > + { > > + /* Skip forward to non-dash on both UUIDs. */ > > + while ('-' == *uuid1) > > + ++uuid1; > > + > > + while ('-' == *uuid2) > > + ++uuid2; > > + > > + if (grub_tolower ((grub_uint8_t) *uuid1) > > + != grub_tolower ((grub_uint8_t) *uuid2)) > > + break; > > + > > + uuid1++; > > + uuid2++; > > + } > > + > > + return (int) grub_tolower ((grub_uint8_t) *uuid1) > > + - (int) grub_tolower ((grub_uint8_t) *uuid2); > > +} > > + > > /* > > * Note that these differ from the C standard's definitions of strtol, > > * strtoul(), and strtoull() by the addition of two const qualifiers on > > the end > > -- > > 2.34.1 > > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel