[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <772a05f5f3c1b35864b80bf78429b843bdf14ee9.camel@linux.ibm.com>
Date: Wed, 08 Mar 2023 15:03:55 +0100
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Arnd Bergmann <arnd@...nel.org>,
kernel test robot <yujie.liu@...el.com>
Cc: oe-lkp@...ts.linux.dev, kernel test robot <lkp@...el.com>,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
Heiko Carstens <hca@...ux.ibm.com>
Subject: Re: [niks:has_ioport_v3] [tty] aa0652d7f1:
BUG:kernel_NULL_pointer_dereference,address
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.
Powered by blists - more mailing lists