linux-kernel - Re: [PATCH 4.19 094/133] USB: serial: iuu

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 21 Jul 2020 13:54:49 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Pavel Machek <pavel@...x.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Johan Hovold <johan@...nel.org>
Subject: Re: [PATCH 4.19 094/133] USB: serial: iuu_phoenix: fix memory
 corruption

On Tue, Jul 21, 2020 at 01:33:00PM +0200, Pavel Machek wrote:
> Hi!
> 
> > commit e7b931bee739e8a77ae216e613d3b99342b6dec0 upstream.
> > 
> > The driver would happily overwrite its write buffer with user data in
> > 256 byte increments due to a removed buffer-space sanity check.
> 
> > +++ b/drivers/usb/serial/iuu_phoenix.c
> > @@ -697,14 +697,16 @@ static int iuu_uart_write(struct tty_str
> >  	struct iuu_private *priv = usb_get_serial_port_data(port);
> >  	unsigned long flags;
> >  
> > -	if (count > 256)
> > -		return -ENOMEM;
> > -
> >  	spin_lock_irqsave(&priv->lock, flags);
> >  
> > +	count = min(count, 256 - priv->writelen);
> > +	if (count == 0)
> > +		goto out;
> > +
> >  	/* fill the buffer */
> >  	memcpy(priv->writebuf + priv->writelen, buf, count);
> >  	priv->writelen += count;
> > +out:
> >  	spin_unlock_irqrestore(&priv->lock, flags);
> >  
> >  	return count;
> 
> Ok, so... goto and label is unneccessary, memcpy will do the right
> thing with count == 0.

That's generally too subtle. Better to clearly mark the error/exception
path.

> But what is worse, this changes return value in the error case;
> returning 0 instead of -ENOMEM. I don't believe 0 is appropriate
> return code here.
> 
> (It should block on the write buffer if blocking or return -EAGAIN if
> nonblocking, right?)

No, zero is the correct return value here when the tty driver's buffer
is full. The line discipline will then handle nonblocking writes
correctly, etc.

Johan

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)