* [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script
@ 2023-02-05 7:05 Glenn Washburn
2023-02-08 18:39 ` Daniel Kiper
0 siblings, 1 reply; 3+ messages in thread
From: Glenn Washburn @ 2023-02-05 7:05 UTC (permalink / raw)
To: grub-devel, Daniel Kiper; +Cc: Patrick Steinhardt, Glenn Washburn
The sector size in bytes is added to each line and it is allowed to be 5
decimal digits long, which covers the most common cases of 512 and 4096
byte sectors with space for an additional digit as future-proofing. The
size allocation is updated to reflect this additional field, allow up to
5 characters and 1 space added.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 34b67a705f..f2c8c8d3fe 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1478,12 +1478,22 @@ luks_script_get (grub_size_t *sz)
*sz = 0;
for (i = cryptodisk_list; i != NULL; i = i->next)
- if (grub_strcmp (i->modname, "luks") == 0)
+ if (grub_strcmp (i->modname, "luks") == 0 ||
+ grub_strcmp (i->modname, "luks2") == 0)
{
- size += sizeof ("luks_mount ");
+ size += grub_strlen (i->modname);
+ size += sizeof ("_mount");
size += grub_strlen (i->uuid);
size += grub_strlen (i->cipher->cipher->name);
- size += 54;
+ /*
+ * Add space in the line for (in order) spaces, cipher mode, cipher IV
+ * mode, sector offset, sector size and the trailing newline. This is
+ * an upper bound on the size of this data. There are 16 extra bytes
+ * in an earlier version of this code that are unaccounted for. It is
+ * left in the calculations in case it is needed. At worst, its short-
+ * lived wasted space.
+ */
+ size += 5 + 5 + 8 + 20 + 5 + 1 + 16;
if (i->essiv_hash)
size += grub_strlen (i->essiv_hash->name);
size += i->keysize * 2;
@@ -1496,16 +1506,18 @@ luks_script_get (grub_size_t *sz)
ptr = ret;
for (i = cryptodisk_list; i != NULL; i = i->next)
- if (grub_strcmp (i->modname, "luks") == 0)
+ if (grub_strcmp (i->modname, "luks") == 0 ||
+ grub_strcmp (i->modname, "luks2") == 0)
{
unsigned j;
const char *iptr;
- ptr = grub_stpcpy (ptr, "luks_mount ");
+ ptr = grub_stpcpy (ptr, i->modname);
+ ptr = grub_stpcpy (ptr, "_mount ");
ptr = grub_stpcpy (ptr, i->uuid);
*ptr++ = ' ';
- grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset_sectors);
- while (*ptr)
- ptr++;
+ ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ",
+ i->offset_sectors);
+ ptr += grub_snprintf (ptr, 7, "%d ", 1 << i->log_sector_size);
for (iptr = i->cipher->cipher->name; *iptr; iptr++)
*ptr++ = grub_tolower (*iptr);
switch (i->mode)
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script
2023-02-05 7:05 [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script Glenn Washburn
@ 2023-02-08 18:39 ` Daniel Kiper
2023-06-28 21:35 ` Glenn Washburn
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Kiper @ 2023-02-08 18:39 UTC (permalink / raw)
To: Glenn Washburn; +Cc: grub-devel, Patrick Steinhardt
On Sun, Feb 05, 2023 at 01:05:31AM -0600, Glenn Washburn wrote:
> The sector size in bytes is added to each line and it is allowed to be 5
> decimal digits long, which covers the most common cases of 512 and 4096
> byte sectors with space for an additional digit as future-proofing. The
> size allocation is updated to reflect this additional field, allow up to
> 5 characters and 1 space added.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 34b67a705f..f2c8c8d3fe 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1478,12 +1478,22 @@ luks_script_get (grub_size_t *sz)
> *sz = 0;
>
> for (i = cryptodisk_list; i != NULL; i = i->next)
> - if (grub_strcmp (i->modname, "luks") == 0)
> + if (grub_strcmp (i->modname, "luks") == 0 ||
> + grub_strcmp (i->modname, "luks2") == 0)
> {
> - size += sizeof ("luks_mount ");
> + size += grub_strlen (i->modname);
> + size += sizeof ("_mount");
> size += grub_strlen (i->uuid);
> size += grub_strlen (i->cipher->cipher->name);
> - size += 54;
> + /*
> + * Add space in the line for (in order) spaces, cipher mode, cipher IV
> + * mode, sector offset, sector size and the trailing newline. This is
> + * an upper bound on the size of this data. There are 16 extra bytes
> + * in an earlier version of this code that are unaccounted for. It is
> + * left in the calculations in case it is needed. At worst, its short-
> + * lived wasted space.
> + */
> + size += 5 + 5 + 8 + 20 + 5 + 1 + 16;
> if (i->essiv_hash)
> size += grub_strlen (i->essiv_hash->name);
> size += i->keysize * 2;
> @@ -1496,16 +1506,18 @@ luks_script_get (grub_size_t *sz)
> ptr = ret;
>
> for (i = cryptodisk_list; i != NULL; i = i->next)
> - if (grub_strcmp (i->modname, "luks") == 0)
> + if (grub_strcmp (i->modname, "luks") == 0 ||
> + grub_strcmp (i->modname, "luks2") == 0)
> {
> unsigned j;
> const char *iptr;
> - ptr = grub_stpcpy (ptr, "luks_mount ");
> + ptr = grub_stpcpy (ptr, i->modname);
> + ptr = grub_stpcpy (ptr, "_mount ");
> ptr = grub_stpcpy (ptr, i->uuid);
> *ptr++ = ' ';
> - grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset_sectors);
> - while (*ptr)
> - ptr++;
> + ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ",
> + i->offset_sectors);
You, OK not you :-), blindly assume offset_sectors is 64-bit long. Today
it is but in the future it may not. May I ask you to define PRIuGRUB_DISK_ADDR,
etc. properly and fix the problem here and in the other places if needed?
> + ptr += grub_snprintf (ptr, 7, "%d ", 1 << i->log_sector_size);
Ugh... Simple "grep -Ir log_sector_size include" revealed we use
different types in different places for log_sector_size. The most
worrying for me are include/grub/disk.h and include/grub/cryptodisk.h.
OK, probably it is not big problem here but I would prefer if we
are consistent all over the place. I think it would be good to do
"typedef grub_uint32_t grub_disk_secs_t" and use grub_disk_secs_t
consistently everywhere. Could you do that? Of course it can be
a separate patch set...
Daniel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script
2023-02-08 18:39 ` Daniel Kiper
@ 2023-06-28 21:35 ` Glenn Washburn
0 siblings, 0 replies; 3+ messages in thread
From: Glenn Washburn @ 2023-06-28 21:35 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel, Patrick Steinhardt
On Wed, 8 Feb 2023 19:39:19 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:
> On Sun, Feb 05, 2023 at 01:05:31AM -0600, Glenn Washburn wrote:
> > The sector size in bytes is added to each line and it is allowed to be 5
> > decimal digits long, which covers the most common cases of 512 and 4096
> > byte sectors with space for an additional digit as future-proofing. The
> > size allocation is updated to reflect this additional field, allow up to
> > 5 characters and 1 space added.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 28 ++++++++++++++++++++--------
> > 1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 34b67a705f..f2c8c8d3fe 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1478,12 +1478,22 @@ luks_script_get (grub_size_t *sz)
> > *sz = 0;
> >
> > for (i = cryptodisk_list; i != NULL; i = i->next)
> > - if (grub_strcmp (i->modname, "luks") == 0)
> > + if (grub_strcmp (i->modname, "luks") == 0 ||
> > + grub_strcmp (i->modname, "luks2") == 0)
> > {
> > - size += sizeof ("luks_mount ");
> > + size += grub_strlen (i->modname);
> > + size += sizeof ("_mount");
> > size += grub_strlen (i->uuid);
> > size += grub_strlen (i->cipher->cipher->name);
> > - size += 54;
> > + /*
> > + * Add space in the line for (in order) spaces, cipher mode, cipher IV
> > + * mode, sector offset, sector size and the trailing newline. This is
> > + * an upper bound on the size of this data. There are 16 extra bytes
> > + * in an earlier version of this code that are unaccounted for. It is
> > + * left in the calculations in case it is needed. At worst, its short-
> > + * lived wasted space.
> > + */
> > + size += 5 + 5 + 8 + 20 + 5 + 1 + 16;
> > if (i->essiv_hash)
> > size += grub_strlen (i->essiv_hash->name);
> > size += i->keysize * 2;
> > @@ -1496,16 +1506,18 @@ luks_script_get (grub_size_t *sz)
> > ptr = ret;
> >
> > for (i = cryptodisk_list; i != NULL; i = i->next)
> > - if (grub_strcmp (i->modname, "luks") == 0)
> > + if (grub_strcmp (i->modname, "luks") == 0 ||
> > + grub_strcmp (i->modname, "luks2") == 0)
> > {
> > unsigned j;
> > const char *iptr;
> > - ptr = grub_stpcpy (ptr, "luks_mount ");
> > + ptr = grub_stpcpy (ptr, i->modname);
> > + ptr = grub_stpcpy (ptr, "_mount ");
> > ptr = grub_stpcpy (ptr, i->uuid);
> > *ptr++ = ' ';
> > - grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ", i->offset_sectors);
> > - while (*ptr)
> > - ptr++;
> > + ptr += grub_snprintf (ptr, 21, "%" PRIuGRUB_UINT64_T " ",
> > + i->offset_sectors);
>
> You, OK not you :-), blindly assume offset_sectors is 64-bit long. Today
> it is but in the future it may not. May I ask you to define PRIuGRUB_DISK_ADDR,
> etc. properly and fix the problem here and in the other places if needed?
Ok, I've added this in a recent patch.
>
> > + ptr += grub_snprintf (ptr, 7, "%d ", 1 << i->log_sector_size);
>
> Ugh... Simple "grep -Ir log_sector_size include" revealed we use
> different types in different places for log_sector_size. The most
> worrying for me are include/grub/disk.h and include/grub/cryptodisk.h.
> OK, probably it is not big problem here but I would prefer if we
> are consistent all over the place. I think it would be good to do
> "typedef grub_uint32_t grub_disk_secs_t" and use grub_disk_secs_t
> consistently everywhere. Could you do that? Of course it can be
> a separate patch set...
I agree consistency is a good thing. In this case, I don't see it
realistically posing any problems. Keeping in mind that what we are
talking about here is the log() base 2 of the sector size, a terabyte
sector size will have a log of 40. This fits in 6 bits. To have a log
sector size larger than 8 bits you're talking about an unbelievably
large number. And this is the sector size we're talking about here. So
we could have this be represented in an 8-bit char and not expect it to
be an issue for at least several centuries.
I'm don't quite see the point of "typedef grub_uint32_t
grub_disk_secs_t" with regards to the log sector size, unless you're
wanting it to be used in conjunction with (1 << log_sector_size). Are
you concerned with that shift overflowing the "%d"?
Glenn
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-28 21:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-05 7:05 [PATCH] luks2: Add support for LUKS2 in (proc)/luks_script Glenn Washburn
2023-02-08 18:39 ` Daniel Kiper
2023-06-28 21:35 ` Glenn Washburn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.