lkml.org 
[lkml]   [2021]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/5] comedi: dt9812: fix DMA buffers on stack
From
Date
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 )=-

\
 
 \ /
  Last update: 2021-10-26 16:37    [W:0.082 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site