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

[Daniel Kiper]

On Sun, Dec 04, 2022 at 01:43:47PM +0000, Maxim Fomin wrote:
> ------- Original Message -------
> On Sunday, December 4th, 2022 at 1:06 PM, 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++;
> > }
> >
> > --
> > 2.38.1
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
> Some comments. This issue was found during work on plainmount
> patch. Previously I tested patch on i386-pc platform only
> partially because 'loopback name (disk,part)offset+' command
> was giving wrong partition sizes. These partition sizes looked
> like there is UINT truncation happening when I boot from my 2
> TB drive. I considered this as a BIOS bug because there are
> other bugs with this laptop (not related to GRUB).
>
> Recently I discovered that exactly the same bug occurs booting
> postUEFI laptop in i386-pc mode on the same 2TB drive. Loopback
> module sets size from 'dev->file->size' (loopback.c:166) which
> is set in file.c:111 by calling '(file->fs->fs_open) (file, file_name)'
> which is blocklist callback (fs.c:137). The root of the bug
> is that the expression 'blocks[i].length << GRUB_DISK_SECTOR_BITS'
> yields low value in i386-pc mode and correct value in x86_64-efi mode.

Patch LGTM. Two paragraphs above should go to the commit message. And
please add your SOB. And repost... :-)

Daniel