Re: [niks:has_ioport_v3] [tty] aa0652d7f1: BUG:kernel_NULL_pointer_dereference,address

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2023-03-08 at 13:21 +0100, Arnd Bergmann wrote:
> On Wed, Mar 8, 2023, at 12:24, Niklas Schnelle wrote:
> > On Thu, 2023-01-05 at 09:03 +0100, Arnd Bergmann wrote:
> > 
> > Yes that makes sense, it's clearly not correct to put the default case
> > inside CONFIG_SERIAL_8250_RT288X. What do you think about going with
> > something like:
> > 
> > @@ -519,9 +534,14 @@ static void set_io_from_upio(struct uart_port *p)
> >  #endif
> > 
> >         default:
> > +#ifdef CONFIG_HAS_IOPORT
> >                 p->serial_in = io_serial_in;
> >                 p->serial_out = io_serial_out;
> >                 break;
> > +#else
> > +               WARN(1, "Unsupported UART type \"io\"\n");
> > +               return;
> > +#endif
> >         }
> 
> I think we have to ensure that ->serial_in() always points
> to some function that doesn't immediately panic, though that
> could be an empty dummy like
> 
>        default:
>                p->serial_in = IS_ENABLED(CONFIG_HAS_IOPORT) ?
>                       io_serial_in : no_serial_in;
>                p->serial_out = IS_ENABLED(CONFIG_HAS_IOPORT) ?
>                       io_serial_out : no_serial_out;

Sadly the IS_ENABLED() plus ternary still gives me an undeclared
identifier error for io_serial_in(). So I think we need the more ugly
#ifdef. With that I hope it would then not crash even if one might be
left without any console at all.

> 
> Ideally we'd make mem_serial_in() the default function
> and only use io_serial_in() when UPIO_PORT is selected,
> but that still causes a NULL pointer dereference when
> a platform initializes a 8250 like
> 
> static struct plat_serial8250_port serial_platform_data[] = {
>         {
>                 .iobase         = 0x3f8, /* NULL pointer */
>                 .irq            = IRQ_ISA_UART,
>                 .uartclk        = 1843200,
>         /* default   .iotype         = UPIO_PORT, */
>         },
> 
> so I think an empty function plus a warning is best here.

So in the above case .iotype is implicitly set to 0 which is UPIO_PORT
so I think one could argue it is selected, no? Not sure how picking
UPIO_MEM as default would look like then. One thing we could do though
is make the switch/case more regular like so:

...
#ifdef CONFIG_HAS_IOPORT
	case UPIO_PORT:
		p->serial_in = io_serial_in;
		p->serial_out = io_serial_out;
		break;
#endif

	default:
		WARN(1, "Unsupported UART type %x\n", p->iotype);
		p->serial_in = no_serial_in;
		p->serial_out = no_serial_out;
	}
...

That way we would have to always define no_serial_in() /
no_serial_out() but would also gracefully handle when p->iotype is set
to some other value and it looks relatively clean.