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

[Avnish Chouhan]

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 
---
 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 (""), "%d", partno);
+  grub_snprintf (optr, sizeof (""), "%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
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Nicholas Vinson]

On 1/12/26 08:29, 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 
---
  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);


I recommend setting the base instead of letting it be 0. An input string 
of "[" would be accepted as the number 43.



+  grub_errno = GRUB_ERR_NONE;


I believe grub_errno should be set before the call to strtoul() instead 
of after it. Setting grub_errno before would allow you to use it as part 
of your error checking below.


Regards,
Nicholas Vinson


+
+  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 (""), "%d", partno);

+  grub_snprintf (optr, sizeof (""), "%lu", partno);
  }
else
  *optr = '\0';



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

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

[Daniel Kiper via Grub-devel]

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./

> Signed-off-by: Avnish Chouhan 
> ---
>  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;

> +  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 (""), "%d", partno);
> +  grub_snprintf (optr, sizeof (""), "%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...

Daniel

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