diff mbox series

[v2,2/2] comedi: dt9812: fix DMA buffers on stack

Message ID 20211027093529.30896-3-johan@kernel.org (mailing list archive)
State Accepted
Commit 536de747bc48262225889a533db6650731ab25d3
Headers show
Series comedi: misc fixes | expand

Commit Message

Johan Hovold Oct. 27, 2021, 9:35 a.m. UTC 
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 | 115 ++++++++++++++++++++++++--------
 1 file changed, 86 insertions(+), 29 deletions(-)

Comments

Ian Abbott Oct. 28, 2021, 9:45 a.m. UTC | #1 
On 27/10/2021 10:35, 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 | 115 ++++++++++++++++++++++++--------
>   1 file changed, 86 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/dt9812.c b/drivers/comedi/drivers/dt9812.c
> index 634f57730c1e..704b04d2980d 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,42 @@ 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;
> +	size_t tbuf_size;
>   	int count, ret;
> +	void *tbuf;
>   
> -	cmd.cmd = cpu_to_le32(DT9812_R_FLASH_DATA);
> -	cmd.u.flash_data_info.address =
> +	tbuf_size = max(sizeof(*cmd), buf_size);
> +
> +	tbuf = kzalloc(tbuf_size, GFP_KERNEL);
> +	if (!tbuf)
> +		return -ENOMEM;
> +
> +	cmd = tbuf;
> +
> +	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);
>   	if (ret)
> -		return ret;
> +		goto out;
> +
> +	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;
> +	}
> +out:
> +	kfree(tbuf);
>   
> -	return usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
> -			    buf, buf_size, &count, DT9812_USB_TIMEOUT);
> +	return ret;
>   }
>   
>   static int dt9812_read_multiple_registers(struct comedi_device *dev,
> @@ -261,22 +282,42 @@ 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;
> +	size_t buf_size;
> +	void *buf;
>   
> -	cmd.cmd = cpu_to_le32(DT9812_R_MULTI_BYTE_REG);
> -	cmd.u.read_multi_info.count = reg_count;
> +	buf_size = max_t(size_t, sizeof(*cmd), reg_count);
> +
> +	buf = kzalloc(buf_size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	cmd = buf;
> +
> +	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);
>   	if (ret)
> -		return ret;
> +		goto out;
> +
> +	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;
> +	}
> +out:
> +	kfree(buf);
>   
> -	return usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
> -			    value, reg_count, &count, DT9812_USB_TIMEOUT);
> +	return ret;
>   }
>   
>   static int dt9812_write_multiple_registers(struct comedi_device *dev,
> @@ -285,19 +326,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 +355,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)
> 

Looks great, thanks!

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>
diff mbox series

Patch

diff --git a/drivers/comedi/drivers/dt9812.c b/drivers/comedi/drivers/dt9812.c
index 634f57730c1e..704b04d2980d 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,42 @@  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;
+	size_t tbuf_size;
 	int count, ret;
+	void *tbuf;
 
-	cmd.cmd = cpu_to_le32(DT9812_R_FLASH_DATA);
-	cmd.u.flash_data_info.address =
+	tbuf_size = max(sizeof(*cmd), buf_size);
+
+	tbuf = kzalloc(tbuf_size, GFP_KERNEL);
+	if (!tbuf)
+		return -ENOMEM;
+
+	cmd = tbuf;
+
+	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);
 	if (ret)
-		return ret;
+		goto out;
+
+	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;
+	}
+out:
+	kfree(tbuf);
 
-	return usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
-			    buf, buf_size, &count, DT9812_USB_TIMEOUT);
+	return ret;
 }
 
 static int dt9812_read_multiple_registers(struct comedi_device *dev,
@@ -261,22 +282,42 @@  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;
+	size_t buf_size;
+	void *buf;
 
-	cmd.cmd = cpu_to_le32(DT9812_R_MULTI_BYTE_REG);
-	cmd.u.read_multi_info.count = reg_count;
+	buf_size = max_t(size_t, sizeof(*cmd), reg_count);
+
+	buf = kzalloc(buf_size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	cmd = buf;
+
+	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);
 	if (ret)
-		return ret;
+		goto out;
+
+	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;
+	}
+out:
+	kfree(buf);
 
-	return usb_bulk_msg(usb, usb_rcvbulkpipe(usb, devpriv->cmd_rd.addr),
-			    value, reg_count, &count, DT9812_USB_TIMEOUT);
+	return ret;
 }
 
 static int dt9812_write_multiple_registers(struct comedi_device *dev,
@@ -285,19 +326,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 +355,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)