Re: [PATCH] usb: gadget: printer: fix infinite loop in printer_read()

[Peter Chen]

On 26-07-01 22:53:20, Melbin K Mathew wrote:
> printer_read() uses the same variable for the requested copy size and
> the number of bytes actually copied to user space. copy_to_user()
> returns the number of bytes not copied, so when it fails to copy
> anything, the computed copied length becomes zero.
> 
> In that case len, buf, current_rx_bytes and current_rx_buf are left
> unchanged. If RX data is available and the user buffer remains
> unwritable, the read loop can repeat indefinitely.
> 
> Track the copied length separately and return -EFAULT, or the number of
> bytes already copied, if an iteration makes no progress.
> 
> Fixes: b185f01a9ab7 ("usb: gadget: printer: factor out f_printer")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Melbin K Mathew <mlbnkm1@xxxxxxxxx>
> ---
> A small userspace model/reproducer is available to maintainers on request.
> 
>  drivers/usb/gadget/function/f_printer.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_printer.c b/drivers/usb/gadget/function/f_printer.c
> index e4f7828ae7..e346e4c26e 100644
> --- a/drivers/usb/gadget/function/f_printer.c
> +++ b/drivers/usb/gadget/function/f_printer.c
> @@ -432,7 +432,7 @@ printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  {
>  	struct printer_dev		*dev = fd->private_data;
>  	unsigned long			flags;
> -	size_t				size;
> +	size_t				size, not_copied, copied;
>  	size_t				bytes_copied;
>  	struct usb_request		*req;
>  	/* This is a pointer to the current USB rx request. */
> @@ -525,14 +525,16 @@ printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  		else
>  			size = len;
>  
> -		size -= copy_to_user(buf, current_rx_buf, size);
> -		bytes_copied += size;
> -		len -= size;
> -		buf += size;
> +		not_copied = copy_to_user(buf, current_rx_buf, size);
> +		copied = size - not_copied;
> +
> +		bytes_copied += copied;
> +		len -= copied;
> +		buf += copied;
>  
>  		spin_lock_irqsave(&dev->lock, flags);
>  
> -		/* We've disconnected or reset so return. */
> +		/* We have disconnected or reset so return. */

Since it is a bug-fix, and goes to stable tree, drop this un-related change.
Otherwise:
Reviewed-by: Peter Chen <peter.chen@xxxxxxxxxx>

Peter

>  		if (dev->reset_printer) {
>  			list_add(&current_rx_req->list, &dev->rx_reqs);
>  			spin_unlock_irqrestore(&dev->lock, flags);
> @@ -543,6 +545,17 @@ printer_read(struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  		if (dev->interface < 0)
>  			goto out_disabled;
>  
> +		if (!copied) {
> +			dev->current_rx_req = current_rx_req;
> +			dev->current_rx_bytes = current_rx_bytes;
> +			dev->current_rx_buf = current_rx_buf;
> +			spin_unlock_irqrestore(&dev->lock, flags);
> +			mutex_unlock(&dev->lock_printer_io);
> +			return bytes_copied ? bytes_copied : -EFAULT;
> +		}
> +
> +		size = copied;
> +
>  		/* If we not returning all the data left in this RX request
>  		 * buffer then adjust the amount of data left in the buffer.
>  		 * Othewise if we are done with this RX request buffer then
> -- 
> 2.39.5
> 
> 

-- 

Thanks,
Peter Chen