[PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Li Zhong]

uart_port_check() will return NULL pointer when state->uart_port is
NULL. Check the return value before dereference it to avoid
null-pointer-dereference error.

Signed-off-by: Li Zhong <floridsleeves@gmail.com>
---
 drivers/tty/serial/serial_core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 12c87cd201a7..760e177166cf 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -194,6 +194,9 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
 	unsigned long page;
 	int retval = 0;
 
+	if (!uport)
+		return -EIO;
+
 	if (uport->type == PORT_UNKNOWN)
 		return 1;
 
@@ -498,6 +501,8 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 	struct ktermios *termios;
 	int hw_stopped;
 
+	if (!uport)
+		return;
 	/*
 	 * If we have no tty, termios, or the port does not exist,
 	 * then we can't set the parameters for this port.
@@ -1045,6 +1050,8 @@ static int uart_get_lsr_info(struct tty_struct *tty,
 	struct uart_port *uport = uart_port_check(state);
 	unsigned int result;
 
+	if (!uport)
+		return -EIO;
 	result = uport->ops->tx_empty(uport);
 
 	/*
-- 
2.25.1

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Andy Shevchenko]

On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
>
> uart_port_check() will return NULL pointer when state->uart_port is
> NULL. Check the return value before dereference it to avoid
> null-pointer-dereference error.

Have you taken the locking into consideration?
If no, please do, if yes, expand your commit message to explain why
the current locking scheme doesn't prevent an error from happening.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Li Zhong]

On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> >
> > uart_port_check() will return NULL pointer when state->uart_port is
> > NULL. Check the return value before dereference it to avoid
> > null-pointer-dereference error.
>
> Have you taken the locking into consideration?
> If no, please do, if yes, expand your commit message to explain why
> the current locking scheme doesn't prevent an error from happening.
>

The locking is taken into consideration but these three checks do not need to
unlock in error-handling because unlock() will be called in the callers. Will
add the comment in v2 patch.

> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Ilpo Järvinen]

On Sun, 28 Aug 2022, Li Zhong wrote:

> On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> > >
> > > uart_port_check() will return NULL pointer when state->uart_port is
> > > NULL. Check the return value before dereference it to avoid
> > > null-pointer-dereference error.
> >
> > Have you taken the locking into consideration?
> > If no, please do, if yes, expand your commit message to explain why
> > the current locking scheme doesn't prevent an error from happening.
> >
> 
> The locking is taken into consideration but these three checks do not need to
> unlock in error-handling because unlock() will be called in the callers. Will
> add the comment in v2 patch.

I think he meant you should indicate why the current locking doesn't cover 
the case you're fixing, not whether this function should call unlock() or 
not.

-- 
 i.

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Li Zhong]

On Mon, Aug 29, 2022 at 12:09 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Sun, 28 Aug 2022, Li Zhong wrote:
>
> > On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> > > >
> > > > uart_port_check() will return NULL pointer when state->uart_port is
> > > > NULL. Check the return value before dereference it to avoid
> > > > null-pointer-dereference error.
> > >
> > > Have you taken the locking into consideration?
> > > If no, please do, if yes, expand your commit message to explain why
> > > the current locking scheme doesn't prevent an error from happening.
> > >
> >
> > The locking is taken into consideration but these three checks do not need to
> > unlock in error-handling because unlock() will be called in the callers. Will
> > add the comment in v2 patch.
>
> I think he meant you should indicate why the current locking doesn't cover
> the case you're fixing, not whether this function should call unlock() or
> not.
>

Thanks for clarifications. The locking does not guarantee the return value of
uart_port_check()  is not NULL. Actually in line 773 of this file
(drivers/tty/serial/serial_core.c), uart_port_check() is also called in
critical section but still there is check on whether the return value is NULL.

> --
>  i.
>

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Ilpo Järvinen]

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

On Mon, 29 Aug 2022, Li Zhong wrote:

> On Mon, Aug 29, 2022 at 12:09 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Sun, 28 Aug 2022, Li Zhong wrote:
> >
> > > On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> > > > >
> > > > > uart_port_check() will return NULL pointer when state->uart_port is
> > > > > NULL. Check the return value before dereference it to avoid
> > > > > null-pointer-dereference error.
> > > >
> > > > Have you taken the locking into consideration?
> > > > If no, please do, if yes, expand your commit message to explain why
> > > > the current locking scheme doesn't prevent an error from happening.
> > > >
> > >
> > > The locking is taken into consideration but these three checks do not need to
> > > unlock in error-handling because unlock() will be called in the callers. Will
> > > add the comment in v2 patch.
> >
> > I think he meant you should indicate why the current locking doesn't cover
> > the case you're fixing, not whether this function should call unlock() or
> > not.
> >
> 
> Thanks for clarifications. The locking does not guarantee the return value of
> uart_port_check()  is not NULL.

Please put such explanation into the commit message like Andy was asking, 
thank you.

And make sure you properly mention what has changed for any new version 
of any patch you send so that Greg don't need to auto-mail you about it 
(and end up ignoring your patch).

> Actually in line 773 of this file
> (drivers/tty/serial/serial_core.c), uart_port_check() is also called in
> critical section but still there is check on whether the return value is NULL.

Existance of such a check elsewhere alone isn't enough to guarantee that 
the check is necessary (and not even that the check in that other place 
would be necessary). You need a deeper analysis than that. I'm not 
claiming its either way here, just pointing out to the direction/details 
you should consider while writing the analysis of the problem.


-- 
 i.

Re: [PATCH v2] drivers/tty/serial: check the return value of uart_port_check()

[Li Zhong]

On Tue, Aug 30, 2022 at 12:20 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 29 Aug 2022, Li Zhong wrote:
>
> > On Mon, Aug 29, 2022 at 12:09 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Sun, 28 Aug 2022, Li Zhong wrote:
> > >
> > > > On Fri, Aug 26, 2022 at 9:01 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > >
> > > > > On Fri, Aug 26, 2022 at 11:38 AM Li Zhong <floridsleeves@gmail.com> wrote:
> > > > > >
> > > > > > uart_port_check() will return NULL pointer when state->uart_port is
> > > > > > NULL. Check the return value before dereference it to avoid
> > > > > > null-pointer-dereference error.
> > > > >
> > > > > Have you taken the locking into consideration?
> > > > > If no, please do, if yes, expand your commit message to explain why
> > > > > the current locking scheme doesn't prevent an error from happening.
> > > > >
> > > >
> > > > The locking is taken into consideration but these three checks do not need to
> > > > unlock in error-handling because unlock() will be called in the callers. Will
> > > > add the comment in v2 patch.
> > >
> > > I think he meant you should indicate why the current locking doesn't cover
> > > the case you're fixing, not whether this function should call unlock() or
> > > not.
> > >
> >
> > Thanks for clarifications. The locking does not guarantee the return value of
> > uart_port_check()  is not NULL.
>
> Please put such explanation into the commit message like Andy was asking,
> thank you.
>

Thanks! I'll add this into the v3 patch.

> And make sure you properly mention what has changed for any new version
> of any patch you send so that Greg don't need to auto-mail you about it
> (and end up ignoring your patch).
>

I'll mention this in the commit message of new patch.

> > Actually in line 773 of this file
> > (drivers/tty/serial/serial_core.c), uart_port_check() is also called in
> > critical section but still there is check on whether the return value is NULL.
>
> Existance of such a check elsewhere alone isn't enough to guarantee that
> the check is necessary (and not even that the check in that other place
> would be necessary). You need a deeper analysis than that. I'm not
> claiming its either way here, just pointing out to the direction/details
> you should consider while writing the analysis of the problem.
>

Thanks for the suggestions. We will try consider more factor into our analysis
tools to reduce false postives.

>
> --
>  i.