linux-kernel - Re: [niks:has_ioport_v3] [tty] aa0652d7f1: BUG:kernel_NULL_pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 08 Mar 2023 12:24:44 +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 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