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

[Niklas Schnelle]

On Thu, 2023-01-05 at 09:03 +0100, Arnd Bergmann wrote:
> On Thu, Jan 5, 2023, at 06:54, kernel test robot wrote:
> > Greeting,
> > 
> > FYI, we noticed BUG:kernel_NULL_pointer_dereference,address due to 
> > commit (built with clang-14):
> > 
> > commit: aa0652d7f1b311e55232a8153522fdaaba0f197a ("tty: serial: handle 
> > HAS_IOPORT dependencies")
> > https://git.kernel.org/cgit/linux/kernel/git/niks/linux.git 
> > has_ioport_v3
> > 
> > in testcase: boot
> > 
> > on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G
> > 
> > caused below changes (please refer to attached dmesg/kmsg for entire 
> > log/backtrace):
> > 
> > 
> > [    2.166733][    T0] calling  univ8250_console_init+0x0/0x30 @ 0
> > [    2.167555][    T0] BUG: kernel NULL pointer dereference, address: 
> > 00000000
> 
> I think it's this bit:
> 
> @@ -508,12 +523,13 @@ static void set_io_from_upio(struct uart_port *p)
>                 up->dl_read = au_serial_dl_read;
>                 up->dl_write = au_serial_dl_write;
>                 break;
> -#endif
> -
> +#ifdef CONFIG_HAS_IOPORT
>         default:
>                 p->serial_in = io_serial_in;
>                 p->serial_out = io_serial_out;
>                 break;
> +#endif
> +#endif
>         }
>         /* Remember loaded iotype */
>         up->cur_iotype = p->iotype;
> 
> 
> which puts the 'default' case inside of '#ifdef
> CONFIG_SERIAL_8250_RT288X'. x86 does not use the
> RT288x variant but relies on the default, so any
> call to io_serial_{in,out} will cause a NULL
> pointer dereference.
> 
>        Arnd

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've pushed a version with the above change rebased on v6.3-rc1 to my
git.kernel.org repository and will do some more testing before I can
hopefully send this out for review and make some progress on this.
Meanwhile the original problem is now the only thing preventing clean
Werror builds on clang for s390 as far as I understand.

Thanks,
Niklas