[PATCH] staging/comedi: Fix undefined array subscript

[PATCH] staging/comedi: Fix undefined array subscript

[Peter Huewe]

In vmk80xx_do_insn_bits the local variable reg, which is used as an
index to the tx_buf array, can be used uninitialized if
- data[0] == 0
and
- devpriv->model != VMK8061_MODEL
-> we get into the else branch without having reg initialized.

Since the driver usually differentiates between VMK8061_MODEL and
VMK8055_MODEL it's safe to assume that VMK8055_DO_REG was meant as an
initial value.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/comedi/drivers/vmk80xx.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c b/drivers/staging/comedi/drivers/vmk80xx.c
index ebf2d48..4ced56a 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -716,6 +716,7 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 			retval = 2;
 		}
 	} else {
+		reg = VMK8055_DO_REG;
 		data[1] = tx_buf[reg];
 		retval = 2;
 	}
-- 
1.7.8.6

Re: [PATCH] staging/comedi: Fix undefined array subscript

[Dan Carpenter]

On Wed, Feb 13, 2013 at 04:30:54AM +0100, Peter Huewe wrote:
> In vmk80xx_do_insn_bits the local variable reg, which is used as an
> index to the tx_buf array, can be used uninitialized if
> - data[0] == 0
> and
> - devpriv->model != VMK8061_MODEL
> -> we get into the else branch without having reg initialized.

It's weird that GCC doesn't warn about this...

This patch works, or at least it doesn't break anything that wasn't
already broken, but it doesn't feel like the right thing.  Probably
we could move the reg setting outside the if statement.

	if (devpriv->model == VMK8055_MODEL) {
		reg = VMK8055_DO_REG;
		cmd = VMK8055_CMD_WRT_AD;
	} else { /* VMK8061_MODEL */
		reg = VMK8061_DO_REG;
		cmd = VMK8061_CMD_DO;
	}

	if (data[0]) {
		tx_buf[reg] &= ~data[0];

Or maybe data[0] == 0 needs to be handled differently.

Ian would know for sure.

regards,
dan carpenter

Re: [PATCH] staging/comedi: Fix undefined array subscript

[Ian Abbott]

On 2013-02-13 07:32, Dan Carpenter wrote:
> On Wed, Feb 13, 2013 at 04:30:54AM +0100, Peter Huewe wrote:
>> In vmk80xx_do_insn_bits the local variable reg, which is used as an
>> index to the tx_buf array, can be used uninitialized if
>> - data[0] == 0
>> and
>> - devpriv->model != VMK8061_MODEL
>> -> we get into the else branch without having reg initialized.
>
> It's weird that GCC doesn't warn about this...
>
> This patch works, or at least it doesn't break anything that wasn't
> already broken, but it doesn't feel like the right thing.  Probably
> we could move the reg setting outside the if statement.
>
> 	if (devpriv->model == VMK8055_MODEL) {
> 		reg = VMK8055_DO_REG;
> 		cmd = VMK8055_CMD_WRT_AD;
> 	} else { /* VMK8061_MODEL */
> 		reg = VMK8061_DO_REG;
> 		cmd = VMK8061_CMD_DO;
> 	}
>
> 	if (data[0]) {
> 		tx_buf[reg] &= ~data[0];

Either method would be fine.

> Or maybe data[0] == 0 needs to be handled differently.
>
> Ian would know for sure.

The insn_bits instruction always reads back the channels after any 
modification of the channels specified by bitmask data[0] with the new 
channel values bitmask data[1].  data[0] == 0 implies you are not 
changing any of the channels so only read back the current values.

For a digital output subdevice, you could either read back the current 
values directly from the hardware or just use the value previously 
written.  The Velleman K8055 doesn't have a command to read back the 
digital outputs from the hardware, so the last written value has to be 
used.  But what if the digital outputs have never been written (or the 
analog outputs have never been written, since the same command updates 
all analog and digital channels)?  A "reset" command is sent to the 
hardware on initialization by vmk80xx_reset_device() (only called for 
the K8055), but I don't know what effect this has on the actual digital 
(and analog) outputs (though I could find out easily enough as we appear 
to have one of these kits (assembled) lying around in the office).  If 
necessary, we may have to also send a "write" command on initialization 
to make the hardware outputs match the initial software state.

For the Velleman K8061, reading the digital outputs from the hardware 
every time is a bit inefficient as it should only be necessary in the 
case where the digital outputs have never been written (similarly for 
the analog outputs).  We could read back the initial state of the 
digital and analog outputs during hardware initialization and keep the 
state up-to-date in software without having to query the hardware again.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging/comedi: Fix undefined array subscript

[Ian Abbott]

On 2013-02-13 11:56, Ian Abbott wrote:
> For a digital output subdevice, you could either read back the current
> values directly from the hardware or just use the value previously
> written.  The Velleman K8055 doesn't have a command to read back the
> digital outputs from the hardware, so the last written value has to be
> used.  But what if the digital outputs have never been written (or the
> analog outputs have never been written, since the same command updates
> all analog and digital channels)?  A "reset" command is sent to the
> hardware on initialization by vmk80xx_reset_device() (only called for
> the K8055), but I don't know what effect this has on the actual digital
> (and analog) outputs (though I could find out easily enough as we appear
> to have one of these kits (assembled) lying around in the office).  If
> necessary, we may have to also send a "write" command on initialization
> to make the hardware outputs match the initial software state.

I've had a quick play with a K8055 and it seems the "reset" command 
issued during hardware initialization has no effect on the digital 
outputs.  (I tested this by setting some digital outputs with comedi 
instructions - there are some handy LEDs on the board that light up or 
not according to the state of the digital outputs - then rmmod'ing and 
modprobe'ing the vmk80xx module - the LEDs remained in the same state.)

Since we can't read back the outputs on this board we should initialize 
them to a known state.  I'll submit some patches later.

Nothing to do with the patch in this thread, which has my Ack.

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

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [PATCH] staging/comedi: Fix undefined array subscript

[Dan Carpenter]

This patch works but it's nasty to re-intialize "reg" inside both
if else statements.  Just do it once at the begining of the
function.

That means we would also delete the ininitialization from the if
side of the if else statement:

 	if (devpriv->model == VMK8061_MODEL) {
-		reg = VMK8061_DO_REG;
 		tx_buf[0] = VMK8061_CMD_RD_DO;

regards,
dan carpenter

[PATCH v2] staging/comedi: Fix undefined array subscript

[Peter Huewe]

In vmk80xx_do_insn_bits the local variable reg, which is used as an
index to the tx_buf array, can be used uninitialized if
- data[0] == 0
and
- devpriv->model != VMK8061_MODEL
-> we get into the else branch without having reg initialized.

Since the driver usually differentiates between VMK8061_MODEL and
VMK8055_MODEL it's safe to assume that VMK8055_DO_REG was meant as an
initial value.

And to avoid duplication we can move the assignments to the top.

Acked-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/staging/comedi/drivers/vmk80xx.c |   17 +++++++----------
 1 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c b/drivers/staging/comedi/drivers/vmk80xx.c
index ebf2d48..eed46ee 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -675,8 +675,14 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 	if (data[0])
 		dir |= DIR_OUT;
 
-	if (devpriv->model == VMK8061_MODEL)
+	if (devpriv->model == VMK8061_MODEL) {
 		dir |= DIR_IN;
+		reg = VMK8061_DO_REG;
+		cmd = VMK8061_CMD_DO;
+	} else { /* VMK8055_MODEL */
+		reg = VMK8055_DO_REG;
+		cmd = VMK8055_CMD_WRT_AD;
+	}
 
 	retval = rudimentary_check(devpriv, dir);
 	if (retval)
@@ -688,14 +694,6 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 	tx_buf = devpriv->usb_tx_buf;
 
 	if (data[0]) {
-		if (devpriv->model == VMK8055_MODEL) {
-			reg = VMK8055_DO_REG;
-			cmd = VMK8055_CMD_WRT_AD;
-		} else { /* VMK8061_MODEL */
-			reg = VMK8061_DO_REG;
-			cmd = VMK8061_CMD_DO;
-		}
-
 		tx_buf[reg] &= ~data[0];
 		tx_buf[reg] |= (data[0] & data[1]);
 
@@ -706,7 +704,6 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 	}
 
 	if (devpriv->model == VMK8061_MODEL) {
-		reg = VMK8061_DO_REG;
 		tx_buf[0] = VMK8061_CMD_RD_DO;
 
 		retval = vmk80xx_read_packet(devpriv);
-- 
1.7.8.6

Re: [PATCH v2] staging/comedi: Fix undefined array subscript

[Dan Carpenter]

Looks great.  :)  Thanks for fixing this bug.

regards,
dan