diff mbox

[v2,media] rc-core: prevent divide by zero bug in s_tx_carrier()

Message ID 20120909203142.GA12296@elgon.mountain (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter Sept. 9, 2012, 8:31 p.m. UTC 
Several of the drivers use carrier as a divisor in their s_tx_carrier()
functions.  We should do a sanity check here like we do for
LIRC_SET_REC_CARRIER.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Ben Hutchings pointed out that my first patch was not a complete
    fix.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Walter Harms Sept. 9, 2012, 9:01 p.m. UTC | #1 
Hi all,
I am not sure if that is a good idea.
it should be in the hands of the driver who to use these 'val'
some driver may need a higher value like this one:

static int iguanair_set_tx_carrier(struct rc_dev *dev, uint32_t carrier)
{
	struct iguanair *ir = dev->priv;

	if (carrier < 25000 || carrier > 150000)
		return -EINVAL;

There are also examples where 0 has a special meaning (to be fair not
with this function). Example:
  cfsetospeed() ...  The zero baud rate, B0, is used to terminate the connection.

I have no clue who will use the 0 but ...

just my 2 cents,
re,
 wh

Am 09.09.2012 22:31, schrieb Dan Carpenter:
> Several of the drivers use carrier as a divisor in their s_tx_carrier()
> functions.  We should do a sanity check here like we do for
> LIRC_SET_REC_CARRIER.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Ben Hutchings pointed out that my first patch was not a complete
>     fix.
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 6ad4a07..28dc0f0 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -211,6 +211,9 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  		if (!dev->s_tx_carrier)
>  			return -EINVAL;
>  
> +		if (val <= 0)
> +			return -EINVAL;
> +
>  		return dev->s_tx_carrier(dev, val);
>  
>  	case LIRC_SET_SEND_DUTY_CYCLE:




--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Sept. 9, 2012, 10 p.m. UTC | #2 
On Sun, 2012-09-09 at 23:01 +0200, walter harms wrote:
> Hi all,
> I am not sure if that is a good idea.
> it should be in the hands of the driver who to use these 'val'
>
> some driver may need a higher value like this one:

I doubt that any driver can actually work with the full range of
positive values, but at least they're less likely to crash.

> static int iguanair_set_tx_carrier(struct rc_dev *dev, uint32_t carrier)
> {
> 	struct iguanair *ir = dev->priv;
> 
> 	if (carrier < 25000 || carrier > 150000)
> 		return -EINVAL;
> 
> There are also examples where 0 has a special meaning (to be fair not
> with this function). Example:
>   cfsetospeed() ...  The zero baud rate, B0, is used to terminate the connection.
>
> I have no clue who will use the 0 but ...
[...]

If an ioctl is defined for a whole class of devices then it is perfectly
valid for the core code for that class to do (some) parameter validation
for the ioctl.  As I'm not really familiar with LIRC I can't say for
sure that 0 is invalid, but if it is then driver writers should not
expect to be able to assign a driver-specific meaning to it.  Consider
what would happen if the LIRC developers wanted to assign a generic
meaning to a value of 0 some time later.

Ben.
Sean Young Sept. 9, 2012, 10:26 p.m. UTC | #3 
On Sun, Sep 09, 2012 at 11:31:42PM +0300, Dan Carpenter wrote:
> Several of the drivers use carrier as a divisor in their s_tx_carrier()
> functions.  We should do a sanity check here like we do for
> LIRC_SET_REC_CARRIER.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: Ben Hutchings pointed out that my first patch was not a complete
>     fix.
> 
> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> index 6ad4a07..28dc0f0 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -211,6 +211,9 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
>  		if (!dev->s_tx_carrier)
>  			return -EINVAL;

This should be ENOSYS.

>  
> +		if (val <= 0)
> +			return -EINVAL;
> +

1) val is unsigned so it would never be less than zero.

2) A value of zero means disabling carrier modulation, which is used by 
   the mceusb driver.

So the check belongs in the individual drivers, as in the original patch.


Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Sept. 9, 2012, 10:38 p.m. UTC | #4 
On Sun, 2012-09-09 at 23:26 +0100, Sean Young wrote:
> On Sun, Sep 09, 2012 at 11:31:42PM +0300, Dan Carpenter wrote:
> > Several of the drivers use carrier as a divisor in their s_tx_carrier()
> > functions.  We should do a sanity check here like we do for
> > LIRC_SET_REC_CARRIER.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > ---
> > v2: Ben Hutchings pointed out that my first patch was not a complete
> >     fix.
> > 
> > diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
> > index 6ad4a07..28dc0f0 100644
> > --- a/drivers/media/rc/ir-lirc-codec.c
> > +++ b/drivers/media/rc/ir-lirc-codec.c
> > @@ -211,6 +211,9 @@ static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
> >  		if (!dev->s_tx_carrier)
> >  			return -EINVAL;
> 
> This should be ENOSYS.
> 
> >  
> > +		if (val <= 0)
> > +			return -EINVAL;
> > +
> 
> 1) val is unsigned so it would never be less than zero.
> 
> 2) A value of zero means disabling carrier modulation, which is used by 
>    the mceusb driver.
>
> So the check belongs in the individual drivers, as in the original patch.

Oh well, sorry for pointing Dan in the wrong direction.  Is the special
case documented somewhere?

Ben.
diff mbox

Patch

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 6ad4a07..28dc0f0 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -211,6 +211,9 @@  static long ir_lirc_ioctl(struct file *filep, unsigned int cmd,
 		if (!dev->s_tx_carrier)
 			return -EINVAL;
 
+		if (val <= 0)
+			return -EINVAL;
+
 		return dev->s_tx_carrier(dev, val);
 
 	case LIRC_SET_SEND_DUTY_CYCLE: