altera_uart: inconsistent checks for null

smatch stuff Dan Carpenter 2 participants 4 messages Feb 19, 2011 – Mar 1, 2011 TTY layer and serial drivers

Share & tools

Dan Carpenter

author

2011-02-19 09:31 UTC #0 lore raw

Hi Tobias,

Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to store bus shift" added a NULL check for platp in altera_uart_probe() but not consistently through out the function.

drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
error: we previously assumed 'platp' could be null.

545 if (platp)

                   ^^^^^^^

checked here.

546 port->regshift = platp->bus_shift;
547 else
548 port->regshift = 0;
549

   550          port->line = i;
   551          port->type = PORT_ALTERA_UART;
   552          port->iotype = SERIAL_IO_MEM;
   553          port->uartclk = platp->uartclk;
                                ^^^^^^^

potential NULL dereference?

   554          port->ops = &altera_uart_ops;

I don't know the right way to address this.

regards,
dan carpenter

Tobias Klauser

2011-02-19 13:22 UTC #1 lore raw

Hi Dan,

Quoted 20 lines · Dan Carpenter

Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to store bus shift" added a NULL check for platp in altera_uart_probe() but not consistently through out the function.
drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
error: we previously assumed 'platp' could be null.
545 if (platp)
                   ^^^^^^^
checked here.
   546                  port->regshift = platp->bus_shift;
   547          else
   548                  port->regshift = 0;
   549
   550          port->line = i;
   551          port->type = PORT_ALTERA_UART;
   552          port->iotype = SERIAL_IO_MEM;
   553          port->uartclk = platp->uartclk;
                                ^^^^^^^

Quoted 1 line

potential NULL dereference?

Not at the moment, because everybody is using platform data at the momenti, so platp should never be NULL. Still this is inconsistent and shouldn't be there like this. The dereference on line 553 will go away with the patch introducing device tree support to altera_uart [1].

[1] https://lkml.org/lkml/2011/2/18/100

Quoted 3 lines

   554          port->ops = &altera_uart_ops;
I don't know the right way to address this.

I guess either Greg or Grant will take the above mentioned patch (it depends on a commit in Grant's tree).

Or do you think we should fix this in another way until the patch is merged?

Thanks a lot for pointing this out
Tobias

Dan Carpenter

author

2011-02-19 14:07 UTC #2 lore raw

Quoted 30 lines · Tobias Klauser

Hi Dan,
Quoted 20 lines · Dan Carpenter
Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to store bus shift" added a NULL check for platp in altera_uart_probe() but not consistently through out the function.
drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
error: we previously assumed 'platp' could be null.
545 if (platp)
                   ^^^^^^^
checked here.
   546                  port->regshift = platp->bus_shift;
   547          else
   548                  port->regshift = 0;
   549
   550          port->line = i;
   551          port->type = PORT_ALTERA_UART;
   552          port->iotype = SERIAL_IO_MEM;
   553          port->uartclk = platp->uartclk;
                                ^^^^^^^
Quoted 1 line
potential NULL dereference?
Not at the moment, because everybody is using platform data at the momenti, so platp should never be NULL. Still this is inconsistent and shouldn't be there like this. The dereference on line 553 will go away with the patch introducing device tree support to altera_uart [1].

Quoted 9 lines

[1] https://lkml.org/lkml/2011/2/18/100
Quoted 3 lines
   554          port->ops = &altera_uart_ops;
I don't know the right way to address this.
I guess either Greg or Grant will take the above mentioned patch (it depends on a commit in Grant's tree).

Sounds good.

regards,
dan carpenter

Tobias Klauser

2011-03-01 08:10 UTC #3 lore raw

Quoted 29 lines · Dan Carpenter

Quoted 28 lines · Tobias Klauser
Quoted 20 lines · Dan Carpenter
Patch 2780ad42f5fe67 "tty: serial: altera_uart: Use port->regshift to store bus shift" added a NULL check for platp in altera_uart_probe() but not consistently through out the function.
drivers/tty/serial/altera_uart.c +553 altera_uart_probe(43)
error: we previously assumed 'platp' could be null.
545 if (platp)
                   ^^^^^^^
checked here.
   546                  port->regshift = platp->bus_shift;
   547          else
   548                  port->regshift = 0;
   549
   550          port->line = i;
   551          port->type = PORT_ALTERA_UART;
   552          port->iotype = SERIAL_IO_MEM;
   553          port->uartclk = platp->uartclk;
                                ^^^^^^^
Quoted 1 line
potential NULL dereference?
Not at the moment, because everybody is using platform data at the momenti, so platp should never be NULL. Still this is inconsistent and shouldn't be there like this. The dereference on line 553 will go away with the patch introducing device tree support to altera_uart [1].

Quoted 8 lines

Quoted 8 lines
[1] https://lkml.org/lkml/2011/2/18/100
Quoted 3 lines
   554          port->ops = &altera_uart_ops;
I don't know the right way to address this.
I guess either Greg or Grant will take the above mentioned patch (it depends on a commit in Grant's tree).

Grant has taken the patch into his tree and it is now in linux-next. I ran smatch against it and the error went away.

Cheers
Tobias