Re: [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform

[Glenn Washburn]

On Sat, 17 Dec 2022 18:22:35 +0000
Maxim Fomin <maxim@fomin.one> wrote:

> From 5db28aa0cb98e906adc7cb735bfa1979ce32c228 Mon Sep 17 00:00:00 2001
> From: Maxim Fomin <maxim@fomin.one>
> Date: Sat, 17 Dec 2022 18:11:34 +0000
> Subject: [PATCH v2 1/1] Fix integer overflow at left shift expression
> on i386-pc platform.
> 
> In case of large partitions (>1TiB) left shift
> expression with unsigned 'length' object and
> signed GRUB_DISK_SECTOR_BITS macro may cause
> integer overflow making calculated partition
> size less than true value. This issue is fixed
> by increasing the size of 'length' integer type
> and casting GRUB_DISK_SECTOR_BITS to unsigned
> type prior to shift expression.
> 
> Signed-off-by: Maxim Fomin <maxim@fomin.one>
> ---
>  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);

I don't know if you saw my response to your V1 patch. I won't repeat
everything here, but suffice to say I think this is unnecessary and it
would be ridiculous if it were necessary. And I don't think it does
what you think it does.

In the C99 spec[1] section 6.5.7, it says "The type of the result is
that of the promoted left operand", which you've just made sure is a
64-bit integer in the first change. Also there you'll see that integer
promotions happen for the left and right operands _individually_. So
this cast doesn't affect the left-hand side. This change really only
does anything if GRUB_DISK_SECTOR_BITS is a negative value. And if
that's the case we've got bigger problems.

>        p++;
>      }
>  

Glenn

[1] https://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf