[PATCH][serial-next] serial: 8250: don't dereference em485 until it has been null checked

[PATCH][serial-next] serial: 8250: don't dereference em485 until it has been null checked

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently, the pointer em485 is dereferenced to get p and then later
em485 is checked to see if it is null before calling __start_tx. In
the case where em485 is null, we get a null pointer dereference. Fix
this by moving the deference and the associated spinlock/unlocks on
p to the code block where em485 is known to be not null.

Detected by CoverityScan, CID#14555001 ("Dereference before null check")

Fixes 6e0a5de2136b ("serial: 8250: Use hrtimers for rs485 delays")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/tty/serial/8250/8250_port.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4726aa276968..c20b581313f0 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1606,18 +1606,18 @@ static inline void start_tx_rs485(struct uart_port *port)
 static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
 {
 	struct uart_8250_em485 *em485;
-	struct uart_8250_port *p;
 	unsigned long flags;
 	em485 = container_of(t, struct uart_8250_em485, start_tx_timer);
-	p = em485->port;
 
-	spin_lock_irqsave(&p->port.lock, flags);
 	if (em485 &&
 	    em485->active_timer == &em485->start_tx_timer) {
+		struct uart_8250_port *p = em485->port;
+
+		spin_lock_irqsave(&p->port.lock, flags);
 		__start_tx(&p->port);
 		em485->active_timer = NULL;
+		spin_unlock_irqrestore(&p->port.lock, flags);
 	}
-	spin_unlock_irqrestore(&p->port.lock, flags);
 	return HRTIMER_NORESTART;
 }
 
-- 
2.14.1

Re: [PATCH][serial-next] serial: 8250: don't dereference em485 until it has been null checked

[Andy Shevchenko]

On Tue, 2017-08-29 at 17:58 +0100, Colin King wrote:

> Currently, the pointer em485 is dereferenced to get p and then later
> em485 is checked to see if it is null before calling __start_tx. In
> the case where em485 is null, we get a null pointer dereference. Fix
> this by moving the deference and the associated spinlock/unlocks on
> p to the code block where em485 is known to be not null.

>  static enum hrtimer_restart serial8250_em485_handle_start_tx(struct
> hrtimer *t)
>  {
>  	struct uart_8250_em485 *em485;
> -	struct uart_8250_port *p;
>  	unsigned long flags;
>  	em485 = container_of(t, struct uart_8250_em485,
> start_tx_timer);
> -	p = em485->port;
>  
> -	spin_lock_irqsave(&p->port.lock, flags);
>  	if (em485 &&
>  	    em485->active_timer == &em485->start_tx_timer) {
> +		struct uart_8250_port *p = em485->port;
> +
> +		spin_lock_irqsave(&p->port.lock, flags);

Can you describe, please, what on your opinion is protected by this
lock?

>  		__start_tx(&p->port);
>  		em485->active_timer = NULL;
> +		spin_unlock_irqrestore(&p->port.lock, flags);
>  	}
> -	spin_unlock_irqrestore(&p->port.lock, flags);
>  	return HRTIMER_NORESTART;
>  }
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH][serial-next] serial: 8250: don't dereference em485 until it has been null checked

[Dan Carpenter]

On Tue, Aug 29, 2017 at 05:58:15PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently, the pointer em485 is dereferenced to get p and then later
> em485 is checked to see if it is null before calling __start_tx. In
> the case where em485 is null, we get a null pointer dereference. Fix
> this by moving the deference and the associated spinlock/unlocks on
> p to the code block where em485 is known to be not null.
> 
> Detected by CoverityScan, CID#14555001 ("Dereference before null check")
> 
> Fixes 6e0a5de2136b ("serial: 8250: Use hrtimers for rs485 delays")

I don't understand which tree this commit is from.  I have it fetched
but when I do a git log on drivers/tty/serial/8250/8250_port.c then I
don't see it.  I have today's linux-next.

> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4726aa276968..c20b581313f0 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1606,18 +1606,18 @@ static inline void start_tx_rs485(struct uart_port *port)
>  static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)

I'm pretty sure "t" isn't ever NULL.

regards,
dan carpenter

Re: [PATCH][serial-next] serial: 8250: don't dereference em485 until it has been null checked

[Andy Shevchenko]

On Tue, 2017-08-29 at 23:04 +0300, Dan Carpenter wrote:
> On Tue, Aug 29, 2017 at 05:58:15PM +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > Currently, the pointer em485 is dereferenced to get p and then later
> > em485 is checked to see if it is null before calling __start_tx. In
> > the case where em485 is null, we get a null pointer dereference. Fix
> > this by moving the deference and the associated spinlock/unlocks on
> > p to the code block where em485 is known to be not null.
> > 
> > Detected by CoverityScan, CID#14555001 ("Dereference before null
> > check")
> > 
> > Fixes 6e0a5de2136b ("serial: 8250: Use hrtimers for rs485 delays")
> 
> I don't understand which tree this commit is from.  I have it fetched
> but when I do a git log on drivers/tty/serial/8250/8250_port.c then I
> don't see it.  I have today's linux-next.

I see it, though I have tty-next as well.


> I'm pretty sure "t" isn't ever NULL.

(I'm pretty sure there is a false positive, though code would be cleaned
to avoid such reports)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy