Re: [PATCH v2 10/10] luks2: Rename source disk variabled named 'disk' to 'source' as in luks.c.

[Glenn Washburn]

On Fri, 9 Oct 2020 12:00:47 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Sat, Oct 03, 2020 at 05:55:34PM -0500, Glenn Washburn wrote:
> > This makes it more obvious to the reader that the disk referred to
> > is the source disk, as opposed to say the disk holding the
> > cryptodisk.
> 
> Hum. I'm not sure this actually helps readability, mostly because I
> think that the distinction here is not that helpful in the context of
> encryption or decryption of the device. In the end we are trying to
> encrypt or decrypt the disk in order to create the new cryptodisk.
> 
> Anyway, I don't particularly care, so take this just as my two cents.
> The patch itself looks good to me.
> 
> Patrick

If I'm following you, you're saying that because encryption is
reversible, `source` is not helpful because either plaintext or
encrypted data can be the source depending on if you're encrypting or
decrypting.

In our case here, I think its intuitive to call the disk `source`
because it is where the data is coming from and to distinguish it from
the cryptodisk grub_disk_t. So its not called source because of the
(encrypted) contents of the disk, as I think you're suggesting. I think
this patch makes more sense in the context of some cryptodisk.c and
luks.c code.

Note that in grub_cryptodisk_scan_device_real in cryptodisk.c
which calls luks2_recover_key the grub_disk_t passed is named `source`.
And in in grub_cryptodisk_open, the parameter `disk` refers to a
grub_disk_t that can be read to decrypt an associated encrypted
grub_disk_t (ie. the cryptodisk grub_disk_t). The grub_cryptodisk_t
associated in disk->data has a member named "source_disk" which points
to the associated grub_disk_t and member "source" which is the name of
the associated disk. In both luks2_decrypt_key and luks2_recover_key the
grub_disk_t argument refers to the encrypted grub_disk_t which can be
accessed as (unencrypted disk)->data->source_disk on the opened crypto
disk. So I think its consistent with cryptodisk.c naming conventions to
call the grub_disk_t argument "source". Also as the subject line says,
this creates consistency with luks.c in its luks2_decrypt_key, which I
suspect is named "source" for the reasons I outlined above. 

I'm a little confused by "In the end we are trying to encrypt or
decrypt the disk in order to create the new cryptodisk." Where does
"encrypt" fit in to creating the new cryptodisk?

> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/luks2.c | 22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 46abc96ef..f918f5a52 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -416,7 +416,7 @@ luks2_verify_key (grub_luks2_digest_t *d,
> > grub_uint8_t *candidate_key, 
> >  static grub_err_t
> >  luks2_decrypt_key (grub_uint8_t *out_key,
> > -              grub_disk_t disk, grub_cryptodisk_t crypt,
> > +              grub_disk_t source, grub_cryptodisk_t crypt,
> >                grub_luks2_keyslot_t *k,
> >                const grub_uint8_t *passphrase, grub_size_t
> > passphraselen) {
> > @@ -492,7 +492,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >      }
> >  
> >    grub_errno = GRUB_ERR_NONE;
> > -  ret = grub_disk_read (disk, 0, k->area.offset, k->area.size,
> > split_key);
> > +  ret = grub_disk_read (source, 0, k->area.offset, k->area.size,
> > split_key); if (ret)
> >      {
> >        grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
> > @@ -536,7 +536,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >  }
> >  
> >  static grub_err_t
> > -luks2_recover_key (grub_disk_t disk,
> > +luks2_recover_key (grub_disk_t source,
> >                grub_cryptodisk_t crypt)
> >  {
> >    grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
> > @@ -551,7 +551,7 @@ luks2_recover_key (grub_disk_t disk,
> >    grub_json_t *json = NULL, keyslots;
> >    grub_err_t ret;
> >  
> > -  ret = luks2_read_header (disk, &header);
> > +  ret = luks2_read_header (source, &header);
> >    if (ret)
> >      return ret;
> >  
> > @@ -560,7 +560,7 @@ luks2_recover_key (grub_disk_t disk,
> >        return GRUB_ERR_OUT_OF_MEMORY;
> >  
> >    /* Read the JSON area. */
> > -  ret = grub_disk_read (disk, 0, grub_be_to_cpu64
> > (header.hdr_offset) + sizeof (header),
> > +  ret = grub_disk_read (source, 0, grub_be_to_cpu64
> > (header.hdr_offset) + sizeof (header), grub_be_to_cpu64
> > (header.hdr_size) - sizeof (header), json_header); if (ret)
> >        goto err;
> > @@ -577,10 +577,10 @@ luks2_recover_key (grub_disk_t disk,
> >      }
> >  
> >    /* Get the passphrase from the user. */
> > -  if (disk->partition)
> > -    part = grub_partition_get_name (disk->partition);
> > -  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
> > disk->name,
> > -           disk->partition ? "," : "", part ? : "",
> > +  if (source->partition)
> > +    part = grub_partition_get_name (source->partition);
> > +  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
> > source->name,
> > +           source->partition ? "," : "", part ? : "",
> >             crypt->uuid);
> >    if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> >      {
> > @@ -616,12 +616,12 @@ luks2_recover_key (grub_disk_t disk,
> >        crypt->log_sector_size = sizeof (unsigned int) * 8
> >             - __builtin_clz ((unsigned int)
> > segment.sector_size) - 1; if (grub_strcmp (segment.size, "dynamic")
> > == 0)
> > -   crypt->total_sectors = (grub_disk_get_size (disk) >>
> > (crypt->log_sector_size - disk->log_sector_size))
> > +   crypt->total_sectors = (grub_disk_get_size (source) >>
> > (crypt->log_sector_size - source->log_sector_size))
> >                            - crypt->offset_sectors;
> >        else
> >     crypt->total_sectors = grub_strtoull (segment.size, NULL,
> > 10) >> crypt->log_sector_size; 
> > -      ret = luks2_decrypt_key (candidate_key, disk, crypt,
> > &keyslot,
> > +      ret = luks2_decrypt_key (candidate_key, source, crypt,
> > &keyslot, (const grub_uint8_t *) passphrase, grub_strlen
> > (passphrase)); if (ret)
> >     {
> > -- 
> > 2.27.0
> >