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
| ||
|
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
Powered by blists - more mailing lists