[PATCH v2] ieee1275 : Add a check for invalid partition number

[PATCH v2] ieee1275 : Add a check for invalid partition number

[Avnish Chouhan]

Adding a check for invalid partition number. grub_strtoul() can fail
in several scenarios like invalid input, overflow, etc will result in
an invalid partition number which could lead to an undefined behavior.

Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
---
 grub-core/kern/ieee1275/openfw.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/ieee1275/openfw.c
index 3b492dd..e82dc34 100644
--- a/grub-core/kern/ieee1275/openfw.c
+++ b/grub-core/kern/ieee1275/openfw.c
@@ -512,7 +512,18 @@ grub_ieee1275_encode_devname (const char *path)
     }
   if (partition && partition[0])
     {
-      unsigned int partno = grub_strtoul (partition, 0, 0);
+      char *endptr;
+      grub_errno = GRUB_ERR_NONE;
+      unsigned int partno = grub_strtoul (partition, &endptr, 0);
+
+      if (grub_errno != GRUB_ERR_NONE || *endptr != '\0')
+        {
+          grub_free (partition);
+          grub_free (device);
+          grub_free (encoding);
+          grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid partition number"));
+          return NULL;
+        }
 
       *optr++ = ',';
 
-- 
2.52.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] ieee1275 : Add a check for invalid partition number

[Daniel Kiper]

On Tue, Dec 02, 2025 at 06:29:44PM +0530, Avnish Chouhan wrote:
> Adding a check for invalid partition number. grub_strtoul() can fail
> in several scenarios like invalid input, overflow, etc will result in
> an invalid partition number which could lead to an undefined behavior.
>
> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> ---
>  grub-core/kern/ieee1275/openfw.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/ieee1275/openfw.c
> index 3b492dd..e82dc34 100644
> --- a/grub-core/kern/ieee1275/openfw.c
> +++ b/grub-core/kern/ieee1275/openfw.c
> @@ -512,7 +512,18 @@ grub_ieee1275_encode_devname (const char *path)
>      }
>    if (partition && partition[0])
>      {
> -      unsigned int partno = grub_strtoul (partition, 0, 0);
> +      char *endptr;
> +      grub_errno = GRUB_ERR_NONE;

You should do this reset after grub_strtoul() call. The commit 533cd4d68
(blsuki: Fix grub_errno leakage in blsuki_is_default_entry()) explains why.

> +      unsigned int partno = grub_strtoul (partition, &endptr, 0);

Do not cast result immediately to shorter type. You are not able to
detect overflow then. First assign result to type size equal to type
returned by the grub_strtoul() function and then check for overflows
properly.

> +      if (grub_errno != GRUB_ERR_NONE || *endptr != '\0')

This check is not reliable. Please take a look at the commit ac8a37dda
(net/http: Allow use of non-standard TCP/IP ports). It shows how it
should be done correctly. Even it is reverted now.

By the way, it would be nice if you could verify correctness of
strtoul()/grub_strtoul()/... calls/checks in the GRUB code after
the release.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] ieee1275 : Add a check for invalid partition number

[Avnish Chouhan]

On 2025-12-20 19:04, Daniel Kiper wrote:
> On Tue, Dec 02, 2025 at 06:29:44PM +0530, Avnish Chouhan wrote:
>> Adding a check for invalid partition number. grub_strtoul() can fail
>> in several scenarios like invalid input, overflow, etc will result in
>> an invalid partition number which could lead to an undefined behavior.
>> 
>> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  grub-core/kern/ieee1275/openfw.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/grub-core/kern/ieee1275/openfw.c 
>> b/grub-core/kern/ieee1275/openfw.c
>> index 3b492dd..e82dc34 100644
>> --- a/grub-core/kern/ieee1275/openfw.c
>> +++ b/grub-core/kern/ieee1275/openfw.c
>> @@ -512,7 +512,18 @@ grub_ieee1275_encode_devname (const char *path)
>>      }
>>    if (partition && partition[0])
>>      {
>> -      unsigned int partno = grub_strtoul (partition, 0, 0);
>> +      char *endptr;
>> +      grub_errno = GRUB_ERR_NONE;
> 
> You should do this reset after grub_strtoul() call. The commit 
> 533cd4d68
> (blsuki: Fix grub_errno leakage in blsuki_is_default_entry()) explains 
> why.
> 

Hi Daniel,
Thank you so much for reviewing the patch!

This I have added so that we'll not catch any earlier errors.
I will reset grub_errno after we verify the grub_strtoul as you 
suggested.

>> +      unsigned int partno = grub_strtoul (partition, &endptr, 0);
> 
> Do not cast result immediately to shorter type. You are not able to
> detect overflow then. First assign result to type size equal to type
> returned by the grub_strtoul() function and then check for overflows
> properly.
> 

Sure. I'll use unsigned long!

>> +      if (grub_errno != GRUB_ERR_NONE || *endptr != '\0')
> 
> This check is not reliable. Please take a look at the commit ac8a37dda
> (net/http: Allow use of non-standard TCP/IP ports). It shows how it
> should be done correctly. Even it is reverted now.
> 

This I have added based on the Alec's suggestion in v1, same as you 
suggested. I have used the same check as in the commit ac8a37dda, just 
skipping the range check due to use of "grub_errno != GRUB_ERR_NONE" 
condition.
Would you like me to add range check here?

> By the way, it would be nice if you could verify correctness of
> strtoul()/grub_strtoul()/... calls/checks in the GRUB code after
> the release.

Sure Daniel, I will surly do!
Thank you!

Regards,
Avnish Chouhan

> 
> Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] ieee1275 : Add a check for invalid partition number

[Daniel Kiper]

On Fri, Jan 02, 2026 at 07:20:06PM +0530, Avnish Chouhan wrote:
> On 2025-12-20 19:04, Daniel Kiper wrote:
> > On Tue, Dec 02, 2025 at 06:29:44PM +0530, Avnish Chouhan wrote:
> > > Adding a check for invalid partition number. grub_strtoul() can fail
> > > in several scenarios like invalid input, overflow, etc will result in
> > > an invalid partition number which could lead to an undefined behavior.
> > >
> > > Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
> > > ---
> > >  grub-core/kern/ieee1275/openfw.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/kern/ieee1275/openfw.c
> > > b/grub-core/kern/ieee1275/openfw.c
> > > index 3b492dd..e82dc34 100644
> > > --- a/grub-core/kern/ieee1275/openfw.c
> > > +++ b/grub-core/kern/ieee1275/openfw.c
> > > @@ -512,7 +512,18 @@ grub_ieee1275_encode_devname (const char *path)
> > >      }
> > >    if (partition && partition[0])
> > >      {
> > > -      unsigned int partno = grub_strtoul (partition, 0, 0);
> > > +      char *endptr;
> > > +      grub_errno = GRUB_ERR_NONE;
> >
> > You should do this reset after grub_strtoul() call. The commit 533cd4d68
> > (blsuki: Fix grub_errno leakage in blsuki_is_default_entry()) explains
> > why.
>
> Hi Daniel,
> Thank you so much for reviewing the patch!

You are welcome!

> This I have added so that we'll not catch any earlier errors.
> I will reset grub_errno after we verify the grub_strtoul as you suggested.

Cool!

> > > +      unsigned int partno = grub_strtoul (partition, &endptr, 0);
> >
> > Do not cast result immediately to shorter type. You are not able to
> > detect overflow then. First assign result to type size equal to type
> > returned by the grub_strtoul() function and then check for overflows
> > properly.
> >
>
> Sure. I'll use unsigned long!

Great!

> > > +      if (grub_errno != GRUB_ERR_NONE || *endptr != '\0')
> >
> > This check is not reliable. Please take a look at the commit ac8a37dda
> > (net/http: Allow use of non-standard TCP/IP ports). It shows how it
> > should be done correctly. Even it is reverted now.
> >
>
> This I have added based on the Alec's suggestion in v1, same as you
> suggested. I have used the same check as in the commit ac8a37dda, just
> skipping the range check due to use of "grub_errno != GRUB_ERR_NONE"
> condition.
> Would you like me to add range check here?

Please check range and drop "grub_errno != GRUB_ERR_NONE" check.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel