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@xxxxxxxxxx>
> > ---
> >   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