linux-kernel - Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std

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 for Android: free password hash cracker in your pocket

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

Message-ID: <424102a6-1f2a-fc18-b1b4-89e00797db26@huawei.com>
Date:   Mon, 20 Jun 2022 16:27:39 +0800
From:   "yiyang (D)" <yiyang13@...wei.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>, <andy.shevchenko@...il.com>,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next] serial: 8250: fix return error code in
 serial8250_request_std_resource()



On 2022/6/20 15:53, Ilpo Järvinen wrote:
> On Mon, 20 Jun 2022, Yi Yang wrote:
> 
>> If port->mapbase = NULL in serial8250_request_std_resource() , it need
>> return a error code instead of 0. If uart_set_info() fail to request new
>> regions by serial8250_request_std_resource() but the return value of
>> serial8250_request_std_resource() is 0, that The system will mistakenly
>> considers that port resources are successfully applied for. A null
>> pointer reference is triggered when the port resource is later invoked.
>>
>> The problem can also be triggered with the following simple program:
>> ----------
>>    #include <stdio.h>
>>    #include <sys/types.h>
>>    #include <sys/stat.h>
>>    #include <fcntl.h>
>>    #include <sys/ioctl.h>
>>    #include <unistd.h>
>>    #include <errno.h>
>>
>>    struct serial_struct {
>>        int type;
>>        int line;
>>        unsigned int    port;
>>        int irq;
>>        int flags;
>>        int xmit_fifo_size;
>>        int custom_divisor;
>>        int baud_base;
>>        unsigned short  close_delay;
>>        char    io_type;
>>        char    reserved_char[1];
>>        int hub6;
>>        unsigned short  closing_wait; /* time to wait before closing */
>>        unsigned short  closing_wait2; /* no longer used... */
>>        unsigned char   *iomem_base;
>>        unsigned short  iomem_reg_shift;
>>        unsigned int    port_high;
>>        unsigned long   iomap_base; /* cookie passed into ioremap */
>>    };
>>
>>    struct serial_struct str;
>>
>>    int main(void)
>>    {
>>        open("/dev/ttyS0", O_RDWR);
>>        ioctl(fd, TIOCGSERIAL, &str);
>>        str.iomem_base = 0;
>>        ioctl(fd, TIOCSSERIAL, str);
>>        return 0;
>>    }
> 
> With admin priviledges I guess?
> 
Yes, this program run on root.
>> ----------
>>
>> Signed-off-by: Yi Yang <yiyang13@...wei.com>
>> ---
>>   drivers/tty/serial/8250/8250_port.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 3e3d784aa628..e1cefa97bdeb 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -2961,8 +2961,10 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
>>   	case UPIO_MEM32BE:
>>   	case UPIO_MEM16:
>>   	case UPIO_MEM:
>> -		if (!port->mapbase)
>> +		if (!port->mapbase) {
>> +			ret = -EFAULT;
>>   			break;
>> +		}
>>   
>>   		if (!request_mem_region(port->mapbase, size, "serial")) {
>>   			ret = -EBUSY;
>>
> 
> I recall reading somewhere that somebody more knowledgeful than me noted
> that this interface has many ways to shoot oneself in the foot if one
> really wants to which is why some things are limited to admin only.
> I cannot seem to find that a reference to that now though.
> 
Maybe some inadvertent behavior can be avoided.

The following code exists in uart_set_info():

If we fail to request resources for the new port, try to restore the old 
settings. But uport->ops->release_port(uport) return value is 0.
----------
         /*
          * Free and release old regions
          */
         if (old_type != PORT_UNKNOWN && uport->ops->release_port)
             uport->ops->release_port(uport);

         uport->iobase = new_port;
         uport->type = new_info->type;
         uport->hub6 = new_info->hub6;
         uport->iotype = new_info->io_type;
         uport->regshift = new_info->iomem_reg_shift;
         uport->mapbase = (unsigned long)new_info->iomem_base;

         /*
          * Claim and map the new regions
          */
         if (uport->type != PORT_UNKNOWN && uport->ops->request_port) {
             retval = uport->ops->request_port(uport);
         } else {
             /* Always success - Jean II */
             retval = 0;
         }

         /*
          * If we fail to request resources for the
          * new port, try to restore the old settings.
          */
         if (retval) {
             uport->iobase = old_iobase;
             uport->type = old_type;
             uport->hub6 = old_hub6;
             uport->iotype = old_iotype;
             uport->regshift = old_shift;
             uport->mapbase = old_mapbase;

             if (old_type != PORT_UNKNOWN) {
                 retval = uport->ops->request_port(uport);
                 /*
                  * If we failed to restore the old settings,
                  * we fail like this.
                  */
                 if (retval)
                     uport->type = PORT_UNKNOWN;

                 /*
                  * We failed anyway.
                  */
                 retval = -EBUSY;
             }
----------

-- 
Yi