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 <developm...@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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
