* [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform
@ 2022-12-17 18:22 Maxim Fomin
2022-12-18 19:25 ` Glenn Washburn
0 siblings, 1 reply; 3+ messages in thread
From: Maxim Fomin @ 2022-12-17 18:22 UTC (permalink / raw)
To: grub-devel; +Cc: dkiper, development
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);
p++;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform
2022-12-17 18:22 [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform Maxim Fomin
@ 2022-12-18 19:25 ` Glenn Washburn
2022-12-19 7:18 ` Maxim Fomin
0 siblings, 1 reply; 3+ messages in thread
From: Glenn Washburn @ 2022-12-18 19:25 UTC (permalink / raw)
To: Maxim Fomin; +Cc: grub-devel, dkiper
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform
2022-12-18 19:25 ` Glenn Washburn
@ 2022-12-19 7:18 ` Maxim Fomin
0 siblings, 0 replies; 3+ messages in thread
From: Maxim Fomin @ 2022-12-19 7:18 UTC (permalink / raw)
To: development; +Cc: grub-devel, dkiper
------- Original Message -------
On Sunday, December 18th, 2022 at 7:25 PM, Glenn Washburn <development@efficientek.com> wrote:
>
> 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
Yes, I saw your response questioning whether cast is necessary. I have
retested both changes (increasing size of struct member and cast) and
indeed cast is unnecessary. I seems I did not reinstalled grub when was
working at the v1 version and this made me think the cast is necessary.
Best regards,
Maxim Fomin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-12-19 7:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-17 18:22 [PATCH v2 1/1] Fix integer overflow at left shift expression on i386-pc platform Maxim Fomin
2022-12-18 19:25 ` Glenn Washburn
2022-12-19 7:18 ` Maxim Fomin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.