linux-kernel - Re: [PATCH] gpio: it87: balance superio enter/exit calls in error path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAMRc=MeJc6+vdvHh7DxXCi5kC_dx555qyqNegOxXJ401S73_uw@mail.gmail.com>
Date: Wed, 10 Dec 2025 06:28:30 +0100
From: Bartosz Golaszewski <brgl@...nel.org>
To: Daniel Gibson <daniel@...son.sh>
Cc: Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>, Linus Walleij <linusw@...nel.org>, 
	Peng Fan <peng.fan@....com>, linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] gpio: it87: balance superio enter/exit calls in error path

On Tue, Dec 9, 2025 at 3:42 PM Daniel Gibson <daniel@...son.sh> wrote:
>
> On 12/9/25 07:12, Bartosz Golaszewski wrote:
> > We always call superio_enter() in it87_gpio_direction_out() but only
> > call superio_exit() if the call to it87_gpio_set() succeeds. Move the
> > label to balance the calls in error path as well.
> >
> > Fixes: ef877a159072 ("gpio: it87: use new line value setter callbacks")
> > Reported-by: Daniel Gibson <daniel@...son.sh>
> > Closes: https://lore.kernel.org/all/bd0a00e3-9b8c-43e8-8772-e67b91f4c71e@gibson.sh/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@....qualcomm.com>
> > ---
> >  drivers/gpio/gpio-it87.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-it87.c b/drivers/gpio/gpio-it87.c
> > index 5d677bcfccf2..528ac1890613 100644
> > --- a/drivers/gpio/gpio-it87.c
> > +++ b/drivers/gpio/gpio-it87.c
> > @@ -254,9 +254,8 @@ static int it87_gpio_direction_out(struct gpio_chip *chip,
> >       if (rc)
> >               goto exit;
> >
> > -     superio_exit();
> > -
> >  exit:
> > +     superio_exit();
> >       spin_unlock(&it87_gpio->lock);
> >       return rc;
> >  }
>
> Moving this after exit: means that superio_exit() is also called if
> superio_enter() failed, a few lines above this patch:
>
>         rc = superio_enter();
>         if (rc)
>                 goto exit;
>
> I don't know if this is really wrong, but it looks fishy and this code
> behaved differently for years - in contrast to the change to skip
> superio_exit() if it87_gpio_set() failed, which is very recent,
> introduced when the return type of it87_gpio_set() was changed from void
> to int.
>
> Probably (again, I don't know if this is actually wrong, I'm not that
> familiar with how this chip works) superio_exit() should remain above
> `exit:` and `if(rc) goto exit;` after `rc = it87_gpio_set(chip,
> gpio_num, val);` should be removed, so:
> - if superio_enter() fails, its error code is returned,
>   but superio_exit() is *not* called
> - the it87_gpio_set(chip, gpio_num, val); return code is returned by
>   it87_gpio_direction_out() no matter if it succeeds or fails,
>   but in either case superio_exit() is called
>

You're probably right, I don't really know this chip either. I will
send a better version.

Bart