[PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Jiri Slaby (SUSE)]

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@kernel.org>
Fixes: 5e227ef2aa38 (serial: uart_poll_init() should power on the UART)
Cc: stable@vger.kernel.org
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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;
 }
 
-- 
2.46.0

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2063 bytes --]

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@kernel.org>
> Fixes: 5e227ef2aa38 (serial: uart_poll_init() should power on the UART)
> Cc: stable@vger.kernel.org
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Doug Anderson]

Hi,

On Mon, Aug 5, 2024 at 7:28 AM Ilpo Järvinen
<ilpo.jarvinen@linux.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@kernel.org>
> > Fixes: 5e227ef2aa38 (serial: uart_poll_init() should power on the UART)
> > Cc: stable@vger.kernel.org
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.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.

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Jiri Slaby]

On 05. 08. 24, 17:46, Doug Anderson wrote:
>>> @@ -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.

The patches got merged. But I can post a fix on top, of course. But, 
what is the consensus here -- include or not to include? I assume 
mutex.h includes cleanup.h already due to the above guard definition.

thanks,
-- 
js
suse labs

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Greg Kroah-Hartman]

On Thu, Aug 08, 2024 at 09:34:33AM +0200, Jiri Slaby wrote:
> On 05. 08. 24, 17:46, Doug Anderson wrote:
> > > > @@ -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.
> 
> The patches got merged. But I can post a fix on top, of course. But, what is
> the consensus here -- include or not to include? I assume mutex.h includes
> cleanup.h already due to the above guard definition.

Leave it as-is, it's not breaking the build anywhere, so not a problem.

thanks,

greg k-h

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Ilpo Järvinen]

On Thu, 8 Aug 2024, Jiri Slaby wrote:

> On 05. 08. 24, 17:46, Doug Anderson wrote:
> > > > @@ -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.
> 
> The patches got merged. But I can post a fix on top, of course. But, what is
> the consensus here -- include or not to include? I assume mutex.h includes
> cleanup.h already due to the above guard definition.

Yeah, while guard() itself is in cleanup.h, Doug has a point that 
DEFINE_GUARD() creates a guaranteed implicit include route for cleanup.h. 
Thus you can disregard my comment as it seems unnecessary to include 
cleanup.h.

-- 
 i.

Re: [PATCH 03/13] serial: don't use uninitialized value in uart_poll_init()

[Doug Anderson]

Hi,

On Mon, Aug 5, 2024 at 3:21 AM Jiri Slaby (SUSE) <jirislaby@kernel.org> 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@kernel.org>
> Fixes: 5e227ef2aa38 (serial: uart_poll_init() should power on the UART)
> Cc: stable@vger.kernel.org
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/tty/serial/serial_core.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

Thanks for the fix! Looks good.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

NOTE: I'm happy to defer to others, but personally I'd consider
breaking this into two changes: one that fixes the problem without
using guard() (which should be pretty simple) and one that switches to
guard(). The issue is that at the time the bug was introduced the
guard() syntax didn't exist and that means backporting will be a bit
of a pain.

Oh, though I guess maybe it doesn't matter since the bug was
introduced in 6.4 and that's not an LTS kernel so nobody cares? ...and
guard() is in 6.6, so maybe things are fine the way you have it.