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

On 2026-01-12 19:19, Daniel Kiper wrote:
> On Mon, Jan 12, 2026 at 06:59:00PM +0530, Avnish Chouhan wrote:
>> Adding a check for invalid partition number. grub_strtoul() can fail
> 
> s/Adding/Add/
> s/can/may/
> 
>> in several scenarios like invalid input, overflow, etc will result in
> 
> s/etc/etc./
> 
>> an invalid partition number which could lead to an undefined behavior.
> 
> s/will result in an invalid partition number which could lead to an
> undefined behavior.
>  /Lack of proper check may lead to unexpected failures in the code 
> further./
> 

Hi Daniel,
Thank you for the review!

I'll change these as suggested.

>> Signed-off-by: Avnish Chouhan <[email protected]>
>> ---
>>  grub-core/kern/ieee1275/openfw.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/grub-core/kern/ieee1275/openfw.c 
>> b/grub-core/kern/ieee1275/openfw.c
>> index 3b492dd..f8ae515 100644
>> --- a/grub-core/kern/ieee1275/openfw.c
>> +++ b/grub-core/kern/ieee1275/openfw.c
>> @@ -512,7 +512,20 @@ grub_ieee1275_encode_devname (const char *path)
>>      }
>>    if (partition && partition[0])
>>      {
>> -      unsigned int partno = grub_strtoul (partition, 0, 0);
>> +      char *endptr;
>> +
>> +      unsigned long partno = grub_strtoul (partition, &endptr, 0);
>> +      grub_errno = GRUB_ERR_NONE;
> 
> I would do this
> 
>   unsigned long partno;
>   char *endptr;
> 
>   partno = grub_strtoul (partition, &endptr, 0);
>   grub_errno = GRUB_ERR_NONE;

Sure. I'll change like this.

> 
>> +      if (*endptr != '\0' || partno > 65535 ||
>> +          (partno == 0 && ! grub_ieee1275_test_flag 
>> (GRUB_IEEE1275_FLAG_0_BASED_PARTITIONS)))
>> +        {
>> +          grub_free (partition);
>> +          grub_free (device);
>> +          grub_free (encoding);
>> +          grub_error (GRUB_ERR_BAD_ARGUMENT, N_("invalid partition 
>> number"));
>> +          return NULL;
>> +        }
>> 
>>        *optr++ = ',';
>> 
>> @@ -520,7 +533,7 @@ grub_ieee1275_encode_devname (const char *path)
>>  	/* GRUB partition 1 is OF partition 0.  */
>>  	partno++;
>> 
>> -      grub_snprintf (optr, sizeof ("XXXXXXXXXXXX"), "%d", partno);
>> +      grub_snprintf (optr, sizeof ("XXXXXXXXXXXX"), "%lu", partno);
> 
> Why is this change part of the patch? Even if it is valid it is 
> logically
> different thing. And I would use "%zu" instead...

This we need due to change from "unsigned int partno" to "unsigned long 
partno". In present code, partno is unsigned int, and we were using the 
same (though in the present code, it is wrong. It must be '%u' instead 
of '%d' anyway). Reason we didn't change this in earlier patch versions.

We'll change it to zu as suggested!

Regards,
Avnish Chouhan

> 
> Daniel

_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel