| Message ID | 20211025114532.4599-3-johan@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | comedi: misc fixes | expand |
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.
On Tue, Oct 26, 2021 at 03:27:13PM +0100, Ian Abbott wrote: > 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.) These small allocations will currently never fail, but if they ever were to, there are other allocations done in the I/O path which would have the same effect if they failed. And if this ever happens, you certainly have bigger problems than worrying about the state of this device. :) That said, I'll see if I can reuse a single buffer without things getting too messy. Johan
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; } static int dt9812_read_multiple_registers(struct comedi_device *dev, @@ -261,22 +281,41 @@ static int dt9812_read_multiple_registers(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 i, count, ret; + u8 *buf; - cmd.cmd = cpu_to_le32(DT9812_R_MULTI_BYTE_REG); - cmd.u.read_multi_info.count = reg_count; + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); + if (!cmd) + return -ENOMEM; + + cmd->cmd = cpu_to_le32(DT9812_R_MULTI_BYTE_REG); + cmd->u.read_multi_info.count = reg_count; for (i = 0; i < reg_count; i++) - cmd.u.read_multi_info.address[i] = address[i]; + cmd->u.read_multi_info.address[i] = address[i]; /* 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), - value, reg_count, &count, DT9812_USB_TIMEOUT); + buf = kmalloc(reg_count, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr), + buf, reg_count, &count, DT9812_USB_TIMEOUT); + if (!ret) { + if (count == reg_count) + memcpy(value, buf, reg_count); + else + ret = -EREMOTEIO; + } + kfree(buf); + + return ret; } static int dt9812_write_multiple_registers(struct comedi_device *dev, @@ -285,19 +324,27 @@ static int dt9812_write_multiple_registers(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 i, count; + int ret; + + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); + if (!cmd) + return -ENOMEM; - cmd.cmd = cpu_to_le32(DT9812_W_MULTI_BYTE_REG); - cmd.u.read_multi_info.count = reg_count; + cmd->cmd = cpu_to_le32(DT9812_W_MULTI_BYTE_REG); + cmd->u.read_multi_info.count = reg_count; for (i = 0; i < reg_count; i++) { - cmd.u.write_multi_info.write[i].address = address[i]; - cmd.u.write_multi_info.write[i].value = value[i]; + cmd->u.write_multi_info.write[i].address = address[i]; + cmd->u.write_multi_info.write[i].value = value[i]; } /* DT9812 only responds to 32 byte writes!! */ - return usb_bulk_msg(usb, usb_sndbulkpipe(usb, devpriv->cmd_wr.addr), - &cmd, 32, &count, DT9812_USB_TIMEOUT); + ret = usb_bulk_msg(usb, usb_sndbulkpipe(usb, devpriv->cmd_wr.addr), + cmd, sizeof(*cmd), &count, DT9812_USB_TIMEOUT); + kfree(cmd); + + return ret; } static int dt9812_rmw_multiple_registers(struct comedi_device *dev, @@ -306,17 +353,25 @@ static int dt9812_rmw_multiple_registers(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 i, count; + int ret; + + cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); + if (!cmd) + return -ENOMEM; - cmd.cmd = cpu_to_le32(DT9812_RMW_MULTI_BYTE_REG); - cmd.u.rmw_multi_info.count = reg_count; + cmd->cmd = cpu_to_le32(DT9812_RMW_MULTI_BYTE_REG); + cmd->u.rmw_multi_info.count = reg_count; for (i = 0; i < reg_count; i++) - cmd.u.rmw_multi_info.rmw[i] = rmw[i]; + cmd->u.rmw_multi_info.rmw[i] = rmw[i]; /* DT9812 only responds to 32 byte writes!! */ - return usb_bulk_msg(usb, usb_sndbulkpipe(usb, devpriv->cmd_wr.addr), - &cmd, 32, &count, DT9812_USB_TIMEOUT); + ret = usb_bulk_msg(usb, usb_sndbulkpipe(usb, devpriv->cmd_wr.addr), + cmd, sizeof(*cmd), &count, DT9812_USB_TIMEOUT); + kfree(cmd); + + return ret; } static int dt9812_digital_in(struct comedi_device *dev, u8 *bits)
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(-)