Messages in this thread | | | Subject | Re: [PATCH 2/5] comedi: dt9812: fix DMA buffers on stack | From | Ian Abbott <> | Date | Tue, 26 Oct 2021 15:27:13 +0100 |
| |
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 )=-
| |