Messages in this thread | | | Subject | Re: [PATCH -next] serial: 8250: fix return error code in serial8250_request_std_resource() | From | "yiyang (D)" <> | Date | Mon, 20 Jun 2022 16:27:39 +0800 |
| |
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
| |