[patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
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.carpen...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
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 15)
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.carpen...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
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 15)
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.
--
Ben Hutchings
Time is nature's way of making sure that everything doesn't happen at once.
signature.asc
Description: This is a digitally signed message part
Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
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.carpen...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
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.carpen...@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. -- Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once. signature.asc Description: This is a digitally signed message part
