Re: [PATCH 2/5] comedi: dt9812: fix DMA buffers on stack

[Ian Abbott <abbotti@mev.co.uk>]

On 25/10/2021 12:45, Johan Hovold wrote:
> USB transfer buffers are typically mapped for DMA and must not be
> allocated on the stack or transfers will fail.
> 
> Allocate proper transfer buffers in the various command helpers and
> return an error on short transfers instead of acting on random stack
> data.
> 
> Note that this also fixes a stack info leak on systems where DMA is not
> used as 32 bytes are always sent to the device regardless of how short
> the command is.
> 
> Fixes: 63274cd7d38a ("Staging: comedi: add usb dt9812 driver")
> Cc: stable@vger.kernel.org      # 2.6.29
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/comedi/drivers/dt9812.c | 109 ++++++++++++++++++++++++--------
>   1 file changed, 82 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/dt9812.c b/drivers/comedi/drivers/dt9812.c
> index 634f57730c1e..f15c306f2d06 100644
> --- a/drivers/comedi/drivers/dt9812.c
> +++ b/drivers/comedi/drivers/dt9812.c
> @@ -32,6 +32,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/errno.h>
> +#include <linux/slab.h>
>   #include <linux/uaccess.h>
>   
>   #include "../comedi_usb.h"
> @@ -237,22 +238,41 @@ static int dt9812_read_info(struct comedi_device *dev,
>   {
>   	struct usb_device *usb = comedi_to_usb_dev(dev);
>   	struct dt9812_private *devpriv = dev->private;
> -	struct dt9812_usb_cmd cmd;
> +	struct dt9812_usb_cmd *cmd;
>   	int count, ret;
> +	u8 *tbuf;
>   
> -	cmd.cmd = cpu_to_le32(DT9812_R_FLASH_DATA);
> -	cmd.u.flash_data_info.address =
> +	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> +	if (!cmd)
> +		return -ENOMEM;
> +
> +	cmd->cmd = cpu_to_le32(DT9812_R_FLASH_DATA);
> +	cmd->u.flash_data_info.address =
>   	    cpu_to_le16(DT9812_DIAGS_BOARD_INFO_ADDR + offset);
> -	cmd.u.flash_data_info.numbytes = cpu_to_le16(buf_size);
> +	cmd->u.flash_data_info.numbytes = cpu_to_le16(buf_size);
>   
>   	/* DT9812 only responds to 32 byte writes!! */
>   	ret = usb_bulk_msg(usb, usb_sndbulkpipe(usb, devpriv->cmd_wr.addr),
> -			   &cmd, 32, &count, DT9812_USB_TIMEOUT);
> +			   cmd, sizeof(*cmd), &count, DT9812_USB_TIMEOUT);
> +	kfree(cmd);
>   	if (ret)
>   		return ret;
>   
> -	return usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
> -			    buf, buf_size, &count, DT9812_USB_TIMEOUT);
> +	tbuf = kmalloc(buf_size, GFP_KERNEL);
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
> +			   tbuf, buf_size, &count, DT9812_USB_TIMEOUT);
> +	if (!ret) {
> +		if (count == buf_size)
> +			memcpy(buf, tbuf, buf_size);
> +		else
> +			ret = -EREMOTEIO;
> +	}
> +	kfree(tbuf);
> +
> +	return ret;
>   }

I suggest doing all the allocations up front so it doesn't leave an 
unread reply message in the unlikely event that the tbuf allocation 
fails.  (It could even allocate a single buffer for both the command and 
the reply since they are not needed at the same time.)

Ditto for the other functions in the patch.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-