[PATCH 1/5] comedi: ni_usb6501: fix NULL-deref in command paths

[PATCH 1/5] comedi: ni_usb6501: fix NULL-deref in command paths

[Johan Hovold]

The driver uses endpoint-sized USB transfer buffers but had no sanity
checks on the sizes. This can lead to zero-size-pointer dereferences or
overflowed transfer buffers in ni6501_port_command() and
ni6501_counter_command() if a (malicious) device has smaller max-packet
sizes than expected (or when doing descriptor fuzz testing).

Add the missing sanity checks to probe().

Fixes: a03bb00e50ab ("staging: comedi: add NI USB-6501 support")
Cc: stable@vger.kernel.org      # 3.18
Cc: Luca Ellero <luca.ellero@brickedbrain.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/comedi/drivers/ni_usb6501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/comedi/drivers/ni_usb6501.c b/drivers/comedi/drivers/ni_usb6501.c
index 5b6d9d783b2f..eb2e5c23f25d 100644
--- a/drivers/comedi/drivers/ni_usb6501.c
+++ b/drivers/comedi/drivers/ni_usb6501.c
@@ -144,6 +144,10 @@ static const u8 READ_COUNTER_RESPONSE[]	= {0x00, 0x01, 0x00, 0x10,
 					   0x00, 0x00, 0x00, 0x02,
 					   0x00, 0x00, 0x00, 0x00};
 
+/* Largest supported packets */
+static const size_t TX_MAX_SIZE	= sizeof(SET_PORT_DIR_REQUEST);
+static const size_t RX_MAX_SIZE	= sizeof(READ_PORT_RESPONSE);
+
 enum commands {
 	READ_PORT,
 	WRITE_PORT,
@@ -486,12 +490,16 @@ static int ni6501_find_endpoints(struct comedi_device *dev)
 		ep_desc = &iface_desc->endpoint[i].desc;
 
 		if (usb_endpoint_is_bulk_in(ep_desc)) {
+			if (usb_endpoint_maxp(ep_desc) < RX_MAX_SIZE)
+				continue;
 			if (!devpriv->ep_rx)
 				devpriv->ep_rx = ep_desc;
 			continue;
 		}
 
 		if (usb_endpoint_is_bulk_out(ep_desc)) {
+			if (usb_endpoint_maxp(ep_desc) < TX_MAX_SIZE)
+				continue;
 			if (!devpriv->ep_tx)
 				devpriv->ep_tx = ep_desc;
 			continue;
-- 
2.32.0

Re: [PATCH 1/5] comedi: ni_usb6501: fix NULL-deref in command paths

[Ian Abbott]

On 25/10/2021 12:45, Johan Hovold wrote:
> The driver uses endpoint-sized USB transfer buffers but had no sanity
> checks on the sizes. This can lead to zero-size-pointer dereferences or
> overflowed transfer buffers in ni6501_port_command() and
> ni6501_counter_command() if a (malicious) device has smaller max-packet
> sizes than expected (or when doing descriptor fuzz testing).
> 
> Add the missing sanity checks to probe().
> 
> Fixes: a03bb00e50ab ("staging: comedi: add NI USB-6501 support")
> Cc: stable@vger.kernel.org      # 3.18
> Cc: Luca Ellero <luca.ellero@brickedbrain.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/comedi/drivers/ni_usb6501.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/comedi/drivers/ni_usb6501.c b/drivers/comedi/drivers/ni_usb6501.c
> index 5b6d9d783b2f..eb2e5c23f25d 100644
> --- a/drivers/comedi/drivers/ni_usb6501.c
> +++ b/drivers/comedi/drivers/ni_usb6501.c
> @@ -144,6 +144,10 @@ static const u8 READ_COUNTER_RESPONSE[]	= {0x00, 0x01, 0x00, 0x10,
>   					   0x00, 0x00, 0x00, 0x02,
>   					   0x00, 0x00, 0x00, 0x00};
>   
> +/* Largest supported packets */
> +static const size_t TX_MAX_SIZE	= sizeof(SET_PORT_DIR_REQUEST);
> +static const size_t RX_MAX_SIZE	= sizeof(READ_PORT_RESPONSE);
> +
>   enum commands {
>   	READ_PORT,
>   	WRITE_PORT,
> @@ -486,12 +490,16 @@ static int ni6501_find_endpoints(struct comedi_device *dev)
>   		ep_desc = &iface_desc->endpoint[i].desc;
>   
>   		if (usb_endpoint_is_bulk_in(ep_desc)) {
> +			if (usb_endpoint_maxp(ep_desc) < RX_MAX_SIZE)
> +				continue;
>   			if (!devpriv->ep_rx)
>   				devpriv->ep_rx = ep_desc;
>   			continue;
>   		}
>   
>   		if (usb_endpoint_is_bulk_out(ep_desc)) {
> +			if (usb_endpoint_maxp(ep_desc) < TX_MAX_SIZE)
> +				continue;
>   			if (!devpriv->ep_tx)
>   				devpriv->ep_tx = ep_desc;
>   			continue;
> 

Perhaps it should return an error if the first encountered bulk-in 
endpoint has the wrong size or the first encountered bulk-out endpoint 
has the wrong size. Something like:

		if (usb_endpoint_is_bulk_in(ep_desc)) {
			if (!devpriv->ep_rx) {
				if (usb_endpoint_maxp(ep_desc) < RX_MAX_SIZE)
					break;
			}
			continue;

(similar for bulk-out with TX_MAX_SIZE)

-- 
-=( 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 )=-

Re: [PATCH 1/5] comedi: ni_usb6501: fix NULL-deref in command paths

[Johan Hovold]

On Tue, Oct 26, 2021 at 03:12:28PM +0100, Ian Abbott wrote:
> On 25/10/2021 12:45, Johan Hovold wrote:
> > The driver uses endpoint-sized USB transfer buffers but had no sanity
> > checks on the sizes. This can lead to zero-size-pointer dereferences or
> > overflowed transfer buffers in ni6501_port_command() and
> > ni6501_counter_command() if a (malicious) device has smaller max-packet
> > sizes than expected (or when doing descriptor fuzz testing).
> > 
> > Add the missing sanity checks to probe().
> > 
> > Fixes: a03bb00e50ab ("staging: comedi: add NI USB-6501 support")
> > Cc: stable@vger.kernel.org      # 3.18
> > Cc: Luca Ellero <luca.ellero@brickedbrain.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >   drivers/comedi/drivers/ni_usb6501.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/comedi/drivers/ni_usb6501.c b/drivers/comedi/drivers/ni_usb6501.c
> > index 5b6d9d783b2f..eb2e5c23f25d 100644
> > --- a/drivers/comedi/drivers/ni_usb6501.c
> > +++ b/drivers/comedi/drivers/ni_usb6501.c
> > @@ -144,6 +144,10 @@ static const u8 READ_COUNTER_RESPONSE[]	= {0x00, 0x01, 0x00, 0x10,
> >   					   0x00, 0x00, 0x00, 0x02,
> >   					   0x00, 0x00, 0x00, 0x00};
> >   
> > +/* Largest supported packets */
> > +static const size_t TX_MAX_SIZE	= sizeof(SET_PORT_DIR_REQUEST);
> > +static const size_t RX_MAX_SIZE	= sizeof(READ_PORT_RESPONSE);
> > +
> >   enum commands {
> >   	READ_PORT,
> >   	WRITE_PORT,
> > @@ -486,12 +490,16 @@ static int ni6501_find_endpoints(struct comedi_device *dev)
> >   		ep_desc = &iface_desc->endpoint[i].desc;
> >   
> >   		if (usb_endpoint_is_bulk_in(ep_desc)) {
> > +			if (usb_endpoint_maxp(ep_desc) < RX_MAX_SIZE)
> > +				continue;
> >   			if (!devpriv->ep_rx)
> >   				devpriv->ep_rx = ep_desc;
> >   			continue;
> >   		}
> >   
> >   		if (usb_endpoint_is_bulk_out(ep_desc)) {
> > +			if (usb_endpoint_maxp(ep_desc) < TX_MAX_SIZE)
> > +				continue;
> >   			if (!devpriv->ep_tx)
> >   				devpriv->ep_tx = ep_desc;
> >   			continue;
> > 
> 
> Perhaps it should return an error if the first encountered bulk-in 
> endpoint has the wrong size or the first encountered bulk-out endpoint 
> has the wrong size. Something like:
> 
> 		if (usb_endpoint_is_bulk_in(ep_desc)) {
> 			if (!devpriv->ep_rx) {
> 				if (usb_endpoint_maxp(ep_desc) < RX_MAX_SIZE)
> 					break;
> 			}
> 			continue;

This is too convoluted, but I can move the max-packet sanity checks
after the endpoint look-ups instead.

It doesn't really matter in the end as the real devices presumably only
have two bulk endpoints.

Johan

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

[Johan Hovold]

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;
 }
 
 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)
-- 
2.32.0

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

[Ian Abbott]

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 )=-

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

[Johan Hovold]

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

[PATCH 3/5] comedi: vmk80xx: fix transfer-buffer overflows

[Johan Hovold]

The driver uses endpoint-sized USB transfer buffers but up until
recently had no sanity checks on the sizes.

Commit e1f13c879a7c ("staging: comedi: check validity of wMaxPacketSize
of usb endpoints found") inadvertently fixed NULL-pointer dereferences
when accessing the transfer buffers in case a malicious device has a
zero wMaxPacketSize.

Make sure to allocate buffers large enough to handle also the other
accesses that are done without a size check (e.g. byte 18 in
vmk80xx_cnt_insn_read() for the VMK8061_MODEL) to avoid writing beyond
the buffers, for example, when doing descriptor fuzzing.

The original driver was for a low-speed device with 8-byte buffers.
Support was later added for a device that uses bulk transfers and is
presumably a full-speed device with a maximum 64-byte wMaxPacketSize.

Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
Cc: stable@vger.kernel.org      # 2.6.31
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/comedi/drivers/vmk80xx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
index 9f920819cd74..f2c1572d0cd7 100644
--- a/drivers/comedi/drivers/vmk80xx.c
+++ b/drivers/comedi/drivers/vmk80xx.c
@@ -90,6 +90,8 @@ enum {
 #define IC3_VERSION		BIT(0)
 #define IC6_VERSION		BIT(1)
 
+#define MIN_BUF_SIZE		64
+
 enum vmk80xx_model {
 	VMK8055_MODEL,
 	VMK8061_MODEL
@@ -678,12 +680,12 @@ static int vmk80xx_alloc_usb_buffers(struct comedi_device *dev)
 	struct vmk80xx_private *devpriv = dev->private;
 	size_t size;
 
-	size = usb_endpoint_maxp(devpriv->ep_rx);
+	size = max(usb_endpoint_maxp(devpriv->ep_rx), MIN_BUF_SIZE);
 	devpriv->usb_rx_buf = kzalloc(size, GFP_KERNEL);
 	if (!devpriv->usb_rx_buf)
 		return -ENOMEM;
 
-	size = usb_endpoint_maxp(devpriv->ep_tx);
+	size = max(usb_endpoint_maxp(devpriv->ep_rx), MIN_BUF_SIZE);
 	devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
 	if (!devpriv->usb_tx_buf)
 		return -ENOMEM;
-- 
2.32.0

Re: [PATCH 3/5] comedi: vmk80xx: fix transfer-buffer overflows

[Ian Abbott]

On 25/10/2021 12:45, Johan Hovold wrote:
> The driver uses endpoint-sized USB transfer buffers but up until
> recently had no sanity checks on the sizes.
> 
> Commit e1f13c879a7c ("staging: comedi: check validity of wMaxPacketSize
> of usb endpoints found") inadvertently fixed NULL-pointer dereferences
> when accessing the transfer buffers in case a malicious device has a
> zero wMaxPacketSize.
> 
> Make sure to allocate buffers large enough to handle also the other
> accesses that are done without a size check (e.g. byte 18 in
> vmk80xx_cnt_insn_read() for the VMK8061_MODEL) to avoid writing beyond
> the buffers, for example, when doing descriptor fuzzing.
> 
> The original driver was for a low-speed device with 8-byte buffers.
> Support was later added for a device that uses bulk transfers and is
> presumably a full-speed device with a maximum 64-byte wMaxPacketSize.
> 
> Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
> Cc: stable@vger.kernel.org      # 2.6.31
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/comedi/drivers/vmk80xx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
> index 9f920819cd74..f2c1572d0cd7 100644
> --- a/drivers/comedi/drivers/vmk80xx.c
> +++ b/drivers/comedi/drivers/vmk80xx.c
> @@ -90,6 +90,8 @@ enum {
>   #define IC3_VERSION		BIT(0)
>   #define IC6_VERSION		BIT(1)
>   
> +#define MIN_BUF_SIZE		64
> +
>   enum vmk80xx_model {
>   	VMK8055_MODEL,
>   	VMK8061_MODEL
> @@ -678,12 +680,12 @@ static int vmk80xx_alloc_usb_buffers(struct comedi_device *dev)
>   	struct vmk80xx_private *devpriv = dev->private;
>   	size_t size;
>   
> -	size = usb_endpoint_maxp(devpriv->ep_rx);
> +	size = max(usb_endpoint_maxp(devpriv->ep_rx), MIN_BUF_SIZE);
>   	devpriv->usb_rx_buf = kzalloc(size, GFP_KERNEL);
>   	if (!devpriv->usb_rx_buf)
>   		return -ENOMEM;
>   
> -	size = usb_endpoint_maxp(devpriv->ep_tx);
> +	size = max(usb_endpoint_maxp(devpriv->ep_rx), MIN_BUF_SIZE);
>   	devpriv->usb_tx_buf = kzalloc(size, GFP_KERNEL);
>   	if (!devpriv->usb_tx_buf)
>   		return -ENOMEM;
> 

Looks good, thanks!

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

-- 
-=( 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 )=-

[PATCH 4/5] comedi: vmk80xx: fix bulk-buffer overflow

[Johan Hovold]

The driver is using endpoint-sized buffers but must not assume that the
tx and rx buffers are of equal size or a malicious device could overflow
the slab-allocated receive buffer when doing bulk transfers.

Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
Cc: stable@vger.kernel.org      # 2.6.31
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/comedi/drivers/vmk80xx.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
index f2c1572d0cd7..9c56918e3b76 100644
--- a/drivers/comedi/drivers/vmk80xx.c
+++ b/drivers/comedi/drivers/vmk80xx.c
@@ -159,22 +159,20 @@ static void vmk80xx_do_bulk_msg(struct comedi_device *dev)
 	__u8 rx_addr;
 	unsigned int tx_pipe;
 	unsigned int rx_pipe;
-	size_t size;
+	size_t tx_size;
+	size_t rx_size;
 
 	tx_addr = devpriv->ep_tx->bEndpointAddress;
 	rx_addr = devpriv->ep_rx->bEndpointAddress;
 	tx_pipe = usb_sndbulkpipe(usb, tx_addr);
 	rx_pipe = usb_rcvbulkpipe(usb, rx_addr);
-
-	/*
-	 * The max packet size attributes of the K8061
-	 * input/output endpoints are identical
-	 */
-	size = usb_endpoint_maxp(devpriv->ep_tx);
+	tx_size = usb_endpoint_maxp(devpriv->ep_tx);
+	rx_size = usb_endpoint_maxp(devpriv->ep_rx);
 
 	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf,
-		     size, NULL, devpriv->ep_tx->bInterval);
-	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, size, NULL, HZ * 10);
+		     tx_size, NULL, devpriv->ep_tx->bInterval);
+
+	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL, HZ * 10);
 }
 
 static int vmk80xx_read_packet(struct comedi_device *dev)
-- 
2.32.0

Re: [PATCH 4/5] comedi: vmk80xx: fix bulk-buffer overflow

[Ian Abbott]

On 25/10/2021 12:45, Johan Hovold wrote:
> The driver is using endpoint-sized buffers but must not assume that the
> tx and rx buffers are of equal size or a malicious device could overflow
> the slab-allocated receive buffer when doing bulk transfers.
> 
> Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
> Cc: stable@vger.kernel.org      # 2.6.31
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/comedi/drivers/vmk80xx.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
> index f2c1572d0cd7..9c56918e3b76 100644
> --- a/drivers/comedi/drivers/vmk80xx.c
> +++ b/drivers/comedi/drivers/vmk80xx.c
> @@ -159,22 +159,20 @@ static void vmk80xx_do_bulk_msg(struct comedi_device *dev)
>   	__u8 rx_addr;
>   	unsigned int tx_pipe;
>   	unsigned int rx_pipe;
> -	size_t size;
> +	size_t tx_size;
> +	size_t rx_size;
>   
>   	tx_addr = devpriv->ep_tx->bEndpointAddress;
>   	rx_addr = devpriv->ep_rx->bEndpointAddress;
>   	tx_pipe = usb_sndbulkpipe(usb, tx_addr);
>   	rx_pipe = usb_rcvbulkpipe(usb, rx_addr);
> -
> -	/*
> -	 * The max packet size attributes of the K8061
> -	 * input/output endpoints are identical
> -	 */
> -	size = usb_endpoint_maxp(devpriv->ep_tx);
> +	tx_size = usb_endpoint_maxp(devpriv->ep_tx);
> +	rx_size = usb_endpoint_maxp(devpriv->ep_rx);
>   
>   	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf,
> -		     size, NULL, devpriv->ep_tx->bInterval);
> -	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, size, NULL, HZ * 10);
> +		     tx_size, NULL, devpriv->ep_tx->bInterval);
> +
> +	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL, HZ * 10);
>   }
>   
>   static int vmk80xx_read_packet(struct comedi_device *dev)
> 

Looks good, thanks!

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

-- 
-=( 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 )=-

[PATCH 5/5] comedi: vmk80xx: fix bulk and interrupt message timeouts

[Johan Hovold]

USB bulk and interrupt message timeouts are specified in milliseconds
and should specifically not vary with CONFIG_HZ.

Note that the bulk-out transfer timeout was set to the endpoint
bInterval value, which should be ignored for bulk endpoints and is
typically set to zero. This meant that a failing bulk-out transfer
would never time out.

Assume that the 10 second timeout used for all other transfers is more
than enough also for the bulk-out endpoint.

Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
Fixes: 951348b37738 ("staging: comedi: vmk80xx: wait for URBs to complete")
Cc: stable@vger.kernel.org      # 2.6.31
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/comedi/drivers/vmk80xx.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
index 9c56918e3b76..4b00a9ea611a 100644
--- a/drivers/comedi/drivers/vmk80xx.c
+++ b/drivers/comedi/drivers/vmk80xx.c
@@ -91,6 +91,7 @@ enum {
 #define IC6_VERSION		BIT(1)
 
 #define MIN_BUF_SIZE		64
+#define PACKET_TIMEOUT		10000	/* ms */
 
 enum vmk80xx_model {
 	VMK8055_MODEL,
@@ -169,10 +170,11 @@ static void vmk80xx_do_bulk_msg(struct comedi_device *dev)
 	tx_size = usb_endpoint_maxp(devpriv->ep_tx);
 	rx_size = usb_endpoint_maxp(devpriv->ep_rx);
 
-	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf,
-		     tx_size, NULL, devpriv->ep_tx->bInterval);
+	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf, tx_size, NULL,
+		     PACKET_TIMEOUT);
 
-	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL, HZ * 10);
+	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL,
+		     PACKET_TIMEOUT);
 }
 
 static int vmk80xx_read_packet(struct comedi_device *dev)
@@ -191,7 +193,7 @@ static int vmk80xx_read_packet(struct comedi_device *dev)
 	pipe = usb_rcvintpipe(usb, ep->bEndpointAddress);
 	return usb_interrupt_msg(usb, pipe, devpriv->usb_rx_buf,
 				 usb_endpoint_maxp(ep), NULL,
-				 HZ * 10);
+				 PACKET_TIMEOUT);
 }
 
 static int vmk80xx_write_packet(struct comedi_device *dev, int cmd)
@@ -212,7 +214,7 @@ static int vmk80xx_write_packet(struct comedi_device *dev, int cmd)
 	pipe = usb_sndintpipe(usb, ep->bEndpointAddress);
 	return usb_interrupt_msg(usb, pipe, devpriv->usb_tx_buf,
 				 usb_endpoint_maxp(ep), NULL,
-				 HZ * 10);
+				 PACKET_TIMEOUT);
 }
 
 static int vmk80xx_reset_device(struct comedi_device *dev)
-- 
2.32.0

Re: [PATCH 5/5] comedi: vmk80xx: fix bulk and interrupt message timeouts

[Ian Abbott]

On 25/10/2021 12:45, Johan Hovold wrote:
> USB bulk and interrupt message timeouts are specified in milliseconds
> and should specifically not vary with CONFIG_HZ.
> 
> Note that the bulk-out transfer timeout was set to the endpoint
> bInterval value, which should be ignored for bulk endpoints and is
> typically set to zero. This meant that a failing bulk-out transfer
> would never time out.
> 
> Assume that the 10 second timeout used for all other transfers is more
> than enough also for the bulk-out endpoint.
> 
> Fixes: 985cafccbf9b ("Staging: Comedi: vmk80xx: Add k8061 support")
> Fixes: 951348b37738 ("staging: comedi: vmk80xx: wait for URBs to complete")
> Cc: stable@vger.kernel.org      # 2.6.31
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>   drivers/comedi/drivers/vmk80xx.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/comedi/drivers/vmk80xx.c b/drivers/comedi/drivers/vmk80xx.c
> index 9c56918e3b76..4b00a9ea611a 100644
> --- a/drivers/comedi/drivers/vmk80xx.c
> +++ b/drivers/comedi/drivers/vmk80xx.c
> @@ -91,6 +91,7 @@ enum {
>   #define IC6_VERSION		BIT(1)
>   
>   #define MIN_BUF_SIZE		64
> +#define PACKET_TIMEOUT		10000	/* ms */
>   
>   enum vmk80xx_model {
>   	VMK8055_MODEL,
> @@ -169,10 +170,11 @@ static void vmk80xx_do_bulk_msg(struct comedi_device *dev)
>   	tx_size = usb_endpoint_maxp(devpriv->ep_tx);
>   	rx_size = usb_endpoint_maxp(devpriv->ep_rx);
>   
> -	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf,
> -		     tx_size, NULL, devpriv->ep_tx->bInterval);
> +	usb_bulk_msg(usb, tx_pipe, devpriv->usb_tx_buf, tx_size, NULL,
> +		     PACKET_TIMEOUT);
>   
> -	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL, HZ * 10);
> +	usb_bulk_msg(usb, rx_pipe, devpriv->usb_rx_buf, rx_size, NULL,
> +		     PACKET_TIMEOUT);
>   }
>   
>   static int vmk80xx_read_packet(struct comedi_device *dev)
> @@ -191,7 +193,7 @@ static int vmk80xx_read_packet(struct comedi_device *dev)
>   	pipe = usb_rcvintpipe(usb, ep->bEndpointAddress);
>   	return usb_interrupt_msg(usb, pipe, devpriv->usb_rx_buf,
>   				 usb_endpoint_maxp(ep), NULL,
> -				 HZ * 10);
> +				 PACKET_TIMEOUT);
>   }
>   
>   static int vmk80xx_write_packet(struct comedi_device *dev, int cmd)
> @@ -212,7 +214,7 @@ static int vmk80xx_write_packet(struct comedi_device *dev, int cmd)
>   	pipe = usb_sndintpipe(usb, ep->bEndpointAddress);
>   	return usb_interrupt_msg(usb, pipe, devpriv->usb_tx_buf,
>   				 usb_endpoint_maxp(ep), NULL,
> -				 HZ * 10);
> +				 PACKET_TIMEOUT);
>   }
>   
>   static int vmk80xx_reset_device(struct comedi_device *dev)
> 

Looks good, thanks!

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

-- 
-=( 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 )=-