Re: [CRYPTO-LUKS v1 03/19] cryptodisk: Incorrect calculation of start sector for grub_disk_read in grub_cryptodisk_read.

[Patrick Steinhardt]

On Fri, Jul 31, 2020 at 07:01:44AM -0500, Glenn Washburn wrote:
> Here dev is a grub_cryptodisk_t and dev->offset is offset in sectors of size
> native to the cryptodisk device. The sector is correctly transformed into
> native grub sector size, but then added to dev->offset which is not
> transformed. It would be nice if the type system would help us with this.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index d8f66e9ef..2791a4870 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -757,8 +757,8 @@ grub_cryptodisk_read (grub_disk_t disk, grub_disk_addr_t 
> sector,
>               size, sector, dev->offset);
>  
>    err = grub_disk_read (dev->source_disk,
> -                     (sector << (disk->log_sector_size
> -                                - GRUB_DISK_SECTOR_BITS)) + dev->offset, 0,
> +                     ((sector + dev->offset) << (disk->log_sector_size
> +                                                - GRUB_DISK_SECTOR_BITS)), 0,
>                       size << disk->log_sector_size, buf);
>    if (err)
>      {

So for LUKS2, we initialize `crypt->offset` with `crypt->offset =
grub_divmod64 (segment.offset, segment.sector_size, NULL)` where
`segment.offset` is the offset in bytes and `segment.sector_size` is the
sector size. So yup, it's in sectors.

For LUKS1, we do `newdev->offset = grub_be_to_cpu32
(header.payloadOffset)`, which also is in sectors of 512 bytes.

So the fix does seem correct to me, but I think it's incomplete as we'd
have to do the same for `grub_cryptodisk_write`.

Out of curiosity: did you test these changes? The offset should always
be a positive number, except with a detached header. So wouldn't we
always have hit this bug if this behaviour was indeed wrong?

Patrick

signature.asc
Description: PGP signature