cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[TJ]

https://savannah.gnu.org/bugs/index.php?45889

Boot disk with 3 LUKS/dm-crypt GPT partitions

(hd0,gpt3) (hd0,gpt4) (hd0,gpt5)

grub is in (hd0,gpt3). The others have a LVM VG each.

Using GRUB_ENABLE_CRYPTODISK=y I deliberately fail the first 
pass-phrase entry to get the rescue environment. I then

cryptomount hd0,gpt3

(crypto0) device is now present and prefix/root are set correctly. I 
insmod some other modules (exploring available functions) and

set debug=cryptodisk

I try to

cryptomount hd0,gpt4
cryptomount hd0,gpt4

and see the message

disk/cryptodisk.c:978: already mounted as crypto0

But ls shows only (crypto0)

With the attached patch the mounts now work:

Attempting to decrypt master key...
Enter passphrase for hd0,gpt3 ( ...UUID...)
Slot 0 opened
<<<< next line comes from temporary grub_dprintf() not included in 
patch >>>>
disk/cryptodisk.c:718: insert 0, source 'hd0,gpt3', id 128, dev_id 0
grub rescue> ls
(hd0) (hd0,gpt5) (hd0,gpt4) (hd0,gpt3) (hd0,gpt2) (hd0,gpt1) (crypto0) 
(proc)
grub rescue> cryptomount hd0,gpt4
Attempting to decrypt master key...
Enter passphrase for hd0,gpt4 (...UUID...)
Slot 0 opened
disk/cryptodisk.c:718: insert 1, source 'hd0,gpt4', id 128, dev_id 0
grub rescue> cryptomount hd0,gpt5
Attempting to decrypt master key...
Enter passphrase for hd0,gpt5 (...UUID...)
Slot 0 opened
disk/cryptodisk.c:718: insert 2, source 'hd0,gpt4', id 128, dev_id 0
grub rescue> insmod lvm
grub rescue> ls
(lvm/VG_OS-x86_64.usr_local) (lvm/VG_OS-ubuntu_15.10_var) 
(lvm/VG_OS-ubuntu_15.10_rootfs) (lvm/VG_DATA-home) (hd0) (hd0,gpt5) 
(hd0,gpt4) (hd0,gpt3) (hd0,gpt2) (hd0,gpt1) (crypto2) (crypto1) 
(crypto0) (proc)

---
  grub-core/disk/cryptodisk.c | 7 ++++++-
  include/grub/cryptodisk.h   | 1 +
  2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 82a3dcb..0e6bc3f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -25,6 +25,7 @@
  #include <grub/fs.h>
  #include <grub/file.h>
  #include <grub/procfs.h>
+#include <grub/partition.h>

  #ifdef GRUB_UTIL
  #include <grub/emu/hostdisk.h>
@@ -718,6 +719,7 @@ grub_cryptodisk_insert (grub_cryptodisk_t newdev, 
const char *name,
    newdev->id = last_cryptodisk_id++;
    newdev->source_id = source->id;
    newdev->source_dev_id = source->dev->id;
+  newdev->partition_number = source->partition ? 
source->partition->number : 0;
    newdev->next = cryptodisk_list;
    cryptodisk_list = newdev;

@@ -740,7 +742,9 @@ grub_cryptodisk_get_by_source_disk (grub_disk_t 
disk)
    grub_cryptodisk_t dev;
    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
      if (dev->source_id == disk->id && dev->source_dev_id == 
disk->dev->id)
-      return dev;
+      if ((disk->partition && disk->partition->number == 
dev->partition_number) ||
+          (!disk->partition && dev->partition_number == 0))
+        return dev;
    return NULL;
  }

@@ -761,6 +765,7 @@ grub_cryptodisk_cheat_insert (grub_cryptodisk_t 
newdev, const char *name,
    newdev->cheat_fd = GRUB_UTIL_FD_INVALID;
    newdev->source_id = source->id;
    newdev->source_dev_id = source->dev->id;
+  newdev->partition_number = source->partition ? 
source->partition->number : 0;
    newdev->id = last_cryptodisk_id++;
    newdev->next = cryptodisk_list;
    cryptodisk_list = newdev;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index f2ad2a7..b638f2e 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -97,6 +97,7 @@ struct grub_cryptodisk
    grub_uint8_t rekey_key[64];
    grub_uint64_t last_rekey;
    int rekey_derived_size;
+  int partition_number;
  };
  typedef struct grub_cryptodisk *grub_cryptodisk_t;

-- 
2.5.0

Re: cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 4362 bytes --]

On 06.09.2015 21:10, TJ wrote:
> https://savannah.gnu.org/bugs/index.php?45889
> 
> Boot disk with 3 LUKS/dm-crypt GPT partitions
> 
> (hd0,gpt3) (hd0,gpt4) (hd0,gpt5)
> 
> grub is in (hd0,gpt3). The others have a LVM VG each.
> 
> Using GRUB_ENABLE_CRYPTODISK=y I deliberately fail the first pass-phrase
> entry to get the rescue environment. I then
> 
> cryptomount hd0,gpt3
> 
> (crypto0) device is now present and prefix/root are set correctly. I
> insmod some other modules (exploring available functions) and
> 
> set debug=cryptodisk
> 
> I try to
> 
> cryptomount hd0,gpt4
> cryptomount hd0,gpt4
> 
> and see the message
> 
> disk/cryptodisk.c:978: already mounted as crypto0
> 
> But ls shows only (crypto0)
> 
> With the attached patch the mounts now work:
> 
> Attempting to decrypt master key...
> Enter passphrase for hd0,gpt3 ( ...UUID...)
> Slot 0 opened
> <<<< next line comes from temporary grub_dprintf() not included in patch
>>>>>
> disk/cryptodisk.c:718: insert 0, source 'hd0,gpt3', id 128, dev_id 0
> grub rescue> ls
> (hd0) (hd0,gpt5) (hd0,gpt4) (hd0,gpt3) (hd0,gpt2) (hd0,gpt1) (crypto0)
> (proc)
> grub rescue> cryptomount hd0,gpt4
> Attempting to decrypt master key...
> Enter passphrase for hd0,gpt4 (...UUID...)
> Slot 0 opened
> disk/cryptodisk.c:718: insert 1, source 'hd0,gpt4', id 128, dev_id 0
> grub rescue> cryptomount hd0,gpt5
> Attempting to decrypt master key...
> Enter passphrase for hd0,gpt5 (...UUID...)
> Slot 0 opened
> disk/cryptodisk.c:718: insert 2, source 'hd0,gpt4', id 128, dev_id 0
> grub rescue> insmod lvm
> grub rescue> ls
> (lvm/VG_OS-x86_64.usr_local) (lvm/VG_OS-ubuntu_15.10_var)
> (lvm/VG_OS-ubuntu_15.10_rootfs) (lvm/VG_DATA-home) (hd0) (hd0,gpt5)
> (hd0,gpt4) (hd0,gpt3) (hd0,gpt2) (hd0,gpt1) (crypto2) (crypto1)
> (crypto0) (proc)
> 
> ---
>  grub-core/disk/cryptodisk.c | 7 ++++++-
>  include/grub/cryptodisk.h   | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 82a3dcb..0e6bc3f 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -25,6 +25,7 @@
>  #include <grub/fs.h>
>  #include <grub/file.h>
>  #include <grub/procfs.h>
> +#include <grub/partition.h>
> 
>  #ifdef GRUB_UTIL
>  #include <grub/emu/hostdisk.h>
> @@ -718,6 +719,7 @@ grub_cryptodisk_insert (grub_cryptodisk_t newdev,
> const char *name,
>    newdev->id = last_cryptodisk_id++;
>    newdev->source_id = source->id;
>    newdev->source_dev_id = source->dev->id;
> +  newdev->partition_number = source->partition ?
> source->partition->number : 0;
>    newdev->next = cryptodisk_list;
>    cryptodisk_list = newdev;
> 
> @@ -740,7 +742,9 @@ grub_cryptodisk_get_by_source_disk (grub_disk_t disk)
>    grub_cryptodisk_t dev;
>    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
>      if (dev->source_id == disk->id && dev->source_dev_id == disk->dev->id)
> -      return dev;
> +      if ((disk->partition && disk->partition->number ==
> dev->partition_number) ||
> +          (!disk->partition && dev->partition_number == 0))
> +        return dev;
Please store and compare partition start, not parition number as the
same partition can be available several times through different partiton
schemes under different numbers. Additionally this allows to use
get_partition_start which already has the logic of handling empty partitions
>    return NULL;
>  }
> 
> @@ -761,6 +765,7 @@ grub_cryptodisk_cheat_insert (grub_cryptodisk_t
> newdev, const char *name,
>    newdev->cheat_fd = GRUB_UTIL_FD_INVALID;
>    newdev->source_id = source->id;
>    newdev->source_dev_id = source->dev->id;
> +  newdev->partition_number = source->partition ?
> source->partition->number : 0;
>    newdev->id = last_cryptodisk_id++;
>    newdev->next = cryptodisk_list;
>    cryptodisk_list = newdev;
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index f2ad2a7..b638f2e 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -97,6 +97,7 @@ struct grub_cryptodisk
>    grub_uint8_t rekey_key[64];
>    grub_uint64_t last_rekey;
>    int rekey_derived_size;
> +  int partition_number;
>  };
>  typedef struct grub_cryptodisk *grub_cryptodisk_t;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[TJ]

On 08-09-2015 17:38, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> On 06.09.2015 21:10, TJ wrote:
>> https://savannah.gnu.org/bugs/index.php?45889
>> +      if ((disk->partition && disk->partition->number ==
>> dev->partition_number) ||
>> +          (!disk->partition && dev->partition_number == 0))
>> +        return dev;
> Please store and compare partition start, not parition number as the
> same partition can be available several times through different 
> partiton
> schemes under different numbers. Additionally this allows to use
> get_partition_start which already has the logic of handling empty 
> partitions

Done and tested. Works perfectly.

------
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 82a3dcb..f4cd81b 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -25,6 +25,7 @@
  #include <grub/fs.h>
  #include <grub/file.h>
  #include <grub/procfs.h>
+#include <grub/partition.h>

  #ifdef GRUB_UTIL
  #include <grub/emu/hostdisk.h>
@@ -718,6 +719,7 @@ grub_cryptodisk_insert (grub_cryptodisk_t newdev, 
const char *name,
    newdev->id = last_cryptodisk_id++;
    newdev->source_id = source->id;
    newdev->source_dev_id = source->dev->id;
+  newdev->partition_start = grub_partition_get_start 
(source->partition);
    newdev->next = cryptodisk_list;
    cryptodisk_list = newdev;

@@ -740,7 +742,9 @@ grub_cryptodisk_get_by_source_disk (grub_disk_t 
disk)
    grub_cryptodisk_t dev;
    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
      if (dev->source_id == disk->id && dev->source_dev_id == 
disk->dev->id)
-      return dev;
+      if ((disk->partition && grub_partition_get_start 
(disk->partition) == dev->partition_start) ||
+          (!disk->partition && dev->partition_start == 0))
+        return dev;
    return NULL;
  }

@@ -761,6 +765,7 @@ grub_cryptodisk_cheat_insert (grub_cryptodisk_t 
newdev, const char *name,
    newdev->cheat_fd = GRUB_UTIL_FD_INVALID;
    newdev->source_id = source->id;
    newdev->source_dev_id = source->dev->id;
+  newdev->partition_start = grub_partition_get_start 
(source->partition);
    newdev->id = last_cryptodisk_id++;
    newdev->next = cryptodisk_list;
    cryptodisk_list = newdev;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index f2ad2a7..32f564a 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -97,6 +97,7 @@ struct grub_cryptodisk
    grub_uint8_t rekey_key[64];
    grub_uint64_t last_rekey;
    int rekey_derived_size;
+  grub_disk_addr_t partition_start;
  };
  typedef struct grub_cryptodisk *grub_cryptodisk_t;

Re: cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[Andrei Borzenkov]

09.09.2015 04:18, TJ пишет:
> On 08-09-2015 17:38, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>> On 06.09.2015 21:10, TJ wrote:
>>> https://savannah.gnu.org/bugs/index.php?45889
>>> +      if ((disk->partition && disk->partition->number ==
>>> dev->partition_number) ||
>>> +          (!disk->partition && dev->partition_number == 0))
>>> +        return dev;
>> Please store and compare partition start, not parition number as the
>> same partition can be available several times through different partiton
>> schemes under different numbers. Additionally this allows to use
>> get_partition_start which already has the logic of handling empty
>> partitions
>
> Done and tested. Works perfectly.
>

Well, should not it also compare disk sizes (grub_disk_get_size)? Also 
grub_partition_get_start cannot differentiate between full disk (start 
== 0) and partition that starts at offset 0.

Re: cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[TJ]

On 11-09-2015 15:11, Andrei Borzenkov wrote:
> 09.09.2015 04:18, TJ пишет:
>> On 08-09-2015 17:38, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>> On 06.09.2015 21:10, TJ wrote:
>>>> https://savannah.gnu.org/bugs/index.php?45889
>>>> +      if ((disk->partition && disk->partition->number ==
>>>> dev->partition_number) ||
>>>> +          (!disk->partition && dev->partition_number == 0))
>>>> +        return dev;
>>> Please store and compare partition start, not parition number as the
>>> same partition can be available several times through different 
>>> partiton
>>> schemes under different numbers. Additionally this allows to use
>>> get_partition_start which already has the logic of handling empty
>>> partitions
>> 
>> Done and tested. Works perfectly.
>> 
> 
> Well, should not it also compare disk sizes (grub_disk_get_size)?
> Also grub_partition_get_start cannot differentiate between full disk
> (start == 0) and partition that starts at offset 0.

My original patch differentiated based on partition_number == 0 
indicating a non-partitioned disk (assuming 1-based partition numbers).

Vladimir asked me to use grub_partition_get_start() due to multiple 
partitioning schemes. I was concerned the function has no concept of an 
error indicator but as it returns 0 when no partitions are found that is 
equivalent (although it could be argued it has dual-use if it is 
possible for a partition to start at sector 0).

In grub_cryptodisk_insert() partition_start == 0 means it is a whole 
disk.

In this if() clause the disk has already been confirmed identical and 
so the only question is whether the cryptodisk is a whole-disk or a 
partition, and if so which partition.

As the starting sector is being stored and that is a unique value 
per-disk, regardless of if there are multiple partition schemes (e.g. 
GPT + Hybrid MBR) the starting sectors will be identical.

If the partition lengths are different (in the multiple partition 
schemes) isn't that a bug in the partitioning and something grub doesn't 
need to concern itself about?

In the event of the partition matching failing the function behaves as 
it has done previously, returning the whole device.

Re: cryptodisk: teach grub_cryptodisk_insert() about partitions (bug #45889)

[Andrei Borzenkov]

I committed your patch. Thank you!

11.09.2015 17:54, TJ пишет:
>
>
> On 11-09-2015 15:11, Andrei Borzenkov wrote:
>> 09.09.2015 04:18, TJ пишет:
>>> On 08-09-2015 17:38, Vladimir 'φ-coder/phcoder' Serbinenko wrote:
>>>> On 06.09.2015 21:10, TJ wrote:
>>>>> https://savannah.gnu.org/bugs/index.php?45889
>>>>> +      if ((disk->partition && disk->partition->number ==
>>>>> dev->partition_number) ||
>>>>> +          (!disk->partition && dev->partition_number == 0))
>>>>> +        return dev;
>>>> Please store and compare partition start, not parition number as the
>>>> same partition can be available several times through different
>>>> partiton
>>>> schemes under different numbers. Additionally this allows to use
>>>> get_partition_start which already has the logic of handling empty
>>>> partitions
>>>
>>> Done and tested. Works perfectly.
>>>
>>
>> Well, should not it also compare disk sizes (grub_disk_get_size)?
>> Also grub_partition_get_start cannot differentiate between full disk
>> (start == 0) and partition that starts at offset 0.
>
> My original patch differentiated based on partition_number == 0
> indicating a non-partitioned disk (assuming 1-based partition numbers).
>
> Vladimir asked me to use grub_partition_get_start() due to multiple
> partitioning schemes. I was concerned the function has no concept of an
> error indicator but as it returns 0 when no partitions are found that is
> equivalent (although it could be argued it has dual-use if it is
> possible for a partition to start at sector 0).
>
> In grub_cryptodisk_insert() partition_start == 0 means it is a whole disk.
>
> In this if() clause the disk has already been confirmed identical and so
> the only question is whether the cryptodisk is a whole-disk or a
> partition, and if so which partition.
>
> As the starting sector is being stored and that is a unique value
> per-disk, regardless of if there are multiple partition schemes (e.g.
> GPT + Hybrid MBR) the starting sectors will be identical.
>
> If the partition lengths are different (in the multiple partition
> schemes) isn't that a bug in the partitioning and something grub doesn't
> need to concern itself about?
>
> In the event of the partition matching failing the function behaves as
> it has done previously, returning the whole device.
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel