linux-kernel - Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=WeekuQXzjk90K8jn=Evn8dMaT1RyctbT7gwEZYYgA9Aw@mail.gmail.com>
Date: Mon, 5 Aug 2024 08:46:06 -0700
From: Doug Anderson <dianders@...omium.org>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	linux-serial <linux-serial@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	stable@...r.kernel.org
Subject: Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

Hi,

On Mon, Aug 5, 2024 at 7:28 AM Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com> wrote:
>
> On Mon, 5 Aug 2024, Jiri Slaby (SUSE) wrote:
>
> > Coverity reports (as CID 1536978) that uart_poll_init() passes
> > uninitialized pm_state to uart_change_pm(). It is in case the first 'if'
> > takes the true branch (does "goto out;").
> >
> > Fix this and simplify the function by simple guard(mutex). The code
> > needs no labels after this at all. And it is pretty clear that the code
> > has not fiddled with pm_state at that point.
> >
> > Signed-off-by: Jiri Slaby (SUSE) <jirislaby@...nel.org>
> > Fixes: 5e227ef2aa38 (serial: uart_poll_init() should power on the UART)
> > Cc: stable@...r.kernel.org
> > Cc: Douglas Anderson <dianders@...omium.org>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > ---
> >  drivers/tty/serial/serial_core.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 3afe77f05abf..d63e9b636e02 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -2690,14 +2690,13 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
> >       int ret = 0;
> >
> >       tport = &state->port;
> > -     mutex_lock(&tport->mutex);
> > +
> > +     guard(mutex)(&tport->mutex);
> >
> >       port = uart_port_check(state);
> >       if (!port || port->type == PORT_UNKNOWN ||
> > -         !(port->ops->poll_get_char && port->ops->poll_put_char)) {
> > -             ret = -1;
> > -             goto out;
> > -     }
> > +         !(port->ops->poll_get_char && port->ops->poll_put_char))
> > +             return -1;
> >
> >       pm_state = state->pm_state;
> >       uart_change_pm(state, UART_PM_STATE_ON);
> > @@ -2717,10 +2716,10 @@ static int uart_poll_init(struct tty_driver *driver, int line, char *options)
> >               ret = uart_set_options(port, NULL, baud, parity, bits, flow);
> >               console_list_unlock();
> >       }
> > -out:
> > +
> >       if (ret)
> >               uart_change_pm(state, pm_state);
> > -     mutex_unlock(&tport->mutex);
> > +
> >       return ret;
> >  }
>
> This too needs #include.

Why? I see in "mutex.h" (which is already included by serial_core.c):

DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))

...so we're using the mutex guard and including the header file that
defines the mutex guard. Seems like it's all legit to me.