Re: [PATCH] Fix integer overflow at left shift expression

[Maxim Fomin]

------- Original Message -------
On Friday, December 16th, 2022 at 5:45 PM, Glenn Washburn 
<development@efficientek.com> wrote:
> 
> On Sun, 04 Dec 2022 13:06:37 +0000
> Maxim Fomin maxim@fomin.one wrote:
> 
> > From db82faafba5e7eccd9fd6c0b7314f7322c1aecbd Mon Sep 17 00:00:00 2001
> > From: Maxim Fomin maxim@fomin.one
> > Date: Sun, 4 Dec 2022 12:05:34 +0000
> > Subject: [PATCH] Fix integer overflow at left shift expression.
> > 
> > In case of large partitions (>1TiB) left shift
> > with signed int GRUB_DISK_SECTOR_BITS macro may
> > cause integer overflow which results in wrong
> > partition size.
> > ---
> > grub-core/kern/fs.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/grub-core/kern/fs.c b/grub-core/kern/fs.c
> > index b9508296d..c196f2bf1 100644
> > --- a/grub-core/kern/fs.c
> > +++ b/grub-core/kern/fs.c
> > @@ -130,7 +130,7 @@ grub_fs_probe (grub_device_t device)
> > struct grub_fs_block
> > {
> > grub_disk_addr_t offset;
> > - unsigned long length;
> > + grub_disk_addr_t length;
> > };
> > 
> > static grub_err_t
> > @@ -195,7 +195,7 @@ grub_fs_blocklist_open (grub_file_t file, const
> > char *name) goto fail;
> > }
> > 
> > - file->size += (blocks[i].length << GRUB_DISK_SECTOR_BITS);
> > + file->size += (blocks[i].length << (grub_disk_addr_t)
> > GRUB_DISK_SECTOR_BITS);
> > p++;
> 
> 
> Is this change actually necessary? You're making sure that
> GRUB_DISK_SECTOR_BITS is treated as a 64-bit integer, but it would be
> crazy for it to even be more than an 8 bit integer. Is there some other
> desirable effect of this?
> 
> > }
> 
> 
> Glenn

Yes, casting GRUB_DISK_SECTOR_BITS is necessary according to my debug
work on this issue. Initially I changed only the type of 'length' struct
member, but it was not enough to fix this bug. Casting is necessary not
only to fit GRUB_DISK_SECTOR_BITS to bigger size integer, but to change
its type from signed to unsigned type. It seems it is necessary because
of integer promotion rules of C.

Best regards,
Maxim Fomin