Re: [PATCH] blsuki: Fix grub_errno leakage in blsuki_is_default_entry()

On Mon, Nov 10, 2025 at 03:55:03PM +0100, Michael Lawnick via Grub-devel wrote:
> 
> 
> Am 10.11.2025 um 08:38 schrieb Michael Chang via Grub-devel:
> > The grub_strtol() call in blsuki_is_default_entry() can set grub_errno
> > to GRUB_ERR_BAD_NUMBER if the input string cannot be converted into any
> > valid digits.
> > 
> > This errno value is currently left uncleared, which can lead to
> > unexpected behavior in subsequent logic that tests the result of a
> > function by checking grub_errno.
> > 
> > Clear grub_errno and return false when GRUB_ERR_BAD_NUMBER is set, as
> > this specific error should be ignored in this context.
> > 
> > Signed-off-by: Michael Chang <[email protected]>
> > ---
> >  grub-core/commands/blsuki.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/grub-core/commands/blsuki.c b/grub-core/commands/blsuki.c
> > index 21d767f05..df25b6fbc 100644
> > --- a/grub-core/commands/blsuki.c
> > +++ b/grub-core/commands/blsuki.c
> > @@ -1510,6 +1510,12 @@ blsuki_is_default_entry (const char *def_entry, grub_blsuki_entry_t *entry, int
> >      return true;
> >  
> >    def_idx = grub_strtol (def_entry, &def_entry_end, 0);
> > +  if (grub_errno == GRUB_ERR_BAD_NUMBER)
> > +    {
> > +      grub_errno = GRUB_ERR_NONE;
> > +      return false;
> > +    }
> > +
> >    if (*def_entry_end != '\0' || def_idx < 0 || def_idx > GRUB_INT_MAX)
> >      return false;
> >  
> This way you'll clear a possible previous GRUB_ERR_BAD_NUMBER
> Shouldn't it be this way:

No, I think this patch does not introduce such behavior change. The
previous error of any kind is not considered an issue, and there was no
intent to preserve them unless grub_strtol() was explicitly surrounded
by grub_error_push()/grub_error_pop(), but apparantly it is not there
before the patch is applied. The error return is always overridden by
the error return code of grub_strtol() if there's any.

>      return true;
> 
> +  old_err = grub_errno;
> +  grub_errno = GRUB_ERR_NONE;
>    def_idx = grub_strtol (def_entry, &def_entry_end, 0);
> +  if (grub_errno == GRUB_ERR_BAD_NUMBER)
> +    {
> +      grub_errno = old_err;
> +      return false;
> +    }
> +    grub_errno = old_err;
> +

IMHO restoring unhandled grub_errno here would still be considered a
bug. It could spread to other unrelated modules or functions which might
interpret it as an unspecified/unknown error and thus disrupt the boot
process in one way or another. This is exactly what the patch trying to
avoid.

Thanks,
Michael


>    if (*def_entry_end != '\0' || def_idx < 0 || def_idx > GRUB_INT_MAX)
>      return false;
> 
> JMTC
> KR Michael
> 
> _______________________________________________
> Grub-devel mailing list
> [email protected]
> https://lists.gnu.org/mailman/listinfo/grub-devel

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