linux-kernel - Re: [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do

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]

Message-ID: <e3617587-7ff1-720c-00ab-434608705652@oracle.com>
Date:   Mon, 26 Apr 2021 18:33:01 +0200
From:   Vegard Nossum <vegard.nossum@...cle.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-serial@...r.kernel.org,
        syzbot+4c7f1a69dfe24c6b3aeb@...kaller.appspotmail.com,
        syzbot+92f32d4e21fb246d31a2@...kaller.appspotmail.com,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com,
        Peter Hurley <peter@...leysoftware.com>,
        Caleb Connolly <caleb@...nolly.tech>
Subject: Re: [PATCH] serial: 8250: fix NULL pointer dereference in
 serial8250_do_startup()

On 2021-04-26 18:17, Greg Kroah-Hartman wrote:
> On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote:
>>   static void set_io_from_upio(struct uart_port *p)
>>   {
>>   	struct uart_8250_port *up = up_to_u8250p(p);
>> @@ -2151,6 +2178,11 @@ int serial8250_do_startup(struct uart_port *port)
>>   	unsigned char lsr, iir;
>>   	int retval;
>>   
>> +	if (WARN_ON_ONCE(needs_membase(port->iotype) && !port->membase))
>> +		return -ENODEV;
>> +	if (WARN_ON_ONCE(needs_iobase(port->iotype) && !port->iobase))
>> +		return -ENODEV;
> 
> These WARN_ON() will still trigger syzbot.  Are you sure you tested this
> and had syzbot verify it?

I tested it locally and the WARN_ON()s don't trigger -- presumably
because serial8250_verify_port() is called from uart_set_info() before
we get to serial8250_do_startup():

         /*
          * Ask the low level driver to verify the settings.
          */
         if (uport->ops->verify_port)
                 retval = uport->ops->verify_port(uport, new_info);

[...]

                 retval = uart_startup(tty, state, 1);

At least, this was my intention. Although now that I look at it again,
it looks like this check may be skipped in some cases; is that what
you're referring to?

I didn't have syzbot verify it -- I thought it would do that when
submitting my patch. Looks like I need to push somewhere and ask syzbot
to test it using this?

#syz test: git://repo/address.git commit-hash

(I assume I can send this privately as long as I use the right
syzbot+...@ To-address?)

Thanks,


Vegard