[PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[Yi Yang]

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;
  }
----------

Signed-off-by: Yi Yang <yiyang13@huawei.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;
-- 
2.17.1

Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[Ilpo Järvinen]

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?

> ----------
> 
> Signed-off-by: Yi Yang <yiyang13@huawei.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.


-- 
 i.

Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[yiyang (D)]

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@huawei.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

Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[Greg Kroah-Hartman]

On Mon, Jun 20, 2022 at 10:53:24AM +0300, 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?
> 
> > ----------
> > 
> > Signed-off-by: Yi Yang <yiyang13@huawei.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.

Yes, what could go wrong with allowing userspace to specify memory
locations that a uart might be located at :)

This stuff should all be "locked down" for any system with untrusted
users.

thanks,

greg k-h

Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()

[Greg KH]

On Mon, Jun 20, 2022 at 03:20:25PM +0800, 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;
>   }
> ----------
> 
> Signed-off-by: Yi Yang <yiyang13@huawei.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;

This not a memory fault, that only gets returned for failures when
copying to/from userspace.

Please return -EINVAL or something like that.

thanks,

greg k-h