lkml.org 
[lkml]   [2022]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource()
From
Date


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

\
 
 \ /
  Last update: 2022-06-20 10:29    [W:0.065 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site