rc-core: prevent divide by zero bug in s_tx_carrier()
Subsystems (4)
PATCH v2 1/1 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.
---
v2: Ben Hutchings pointed out that my first patch was not a complete
fix.
drivers/media/rc/ir-lirc-codec.c +3 -0
@@ -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 :
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:
Quoted 23 lines
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 forLIRC_SET_REC_CARRIER.Signed-off-by Dan Carpenter <[email protected]>---
v2: Ben Hutchings pointed out that my first patch was not a complete
fix.drivers/media/rc/ir-lirc-codec.c +3
-0@@ -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 :
Quoted 5 lines · walter harms
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.
Quoted 7 lines
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;
Quoted 5 lines
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.
Signature · 2 lines
Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once.
Sean Young
maintainerQuoted 14 lines · 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 forLIRC_SET_REC_CARRIER.Signed-off-by Dan Carpenter <[email protected]>---
v2: Ben Hutchings pointed out that my first patch was not a complete
fix.drivers/media/rc/ir-lirc-codec.c +0
-0
drivers/media/rc/ir-lirc-codec.c static long ir_lirc_ioctl(struct file *filep, unsigned int cmd, · line 212
Quoted 2 lines
drivers/media/rc/ir-lirc-codec.c +0
-0@@ -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.
drivers/media/rc/ir-lirc-codec.c static long ir_lirc_ioctl(struct file *filep, unsigned int cmd, · line 216
Quoted 4 lines
drivers/media/rc/ir-lirc-codec.c +3
-0+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
Quoted 27 lines · Sean Young
Quoted 14 lines · 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 forLIRC_SET_REC_CARRIER.Signed-off-by Dan Carpenter <[email protected]>---
v2: Ben Hutchings pointed out that my first patch was not a complete
fix.drivers/media/rc/ir-lirc-codec.c +0
-0drivers/media/rc/ir-lirc-codec.c static long ir_lirc_ioctl(struct file *filep, unsigned int cmd, · line 212
Quoted 2 lines
drivers/media/rc/ir-lirc-codec.c +0
-0@@ -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.
drivers/media/rc/ir-lirc-codec.c static long ir_lirc_ioctl(struct file *filep, unsigned int cmd, · line 216
Quoted 4 lines
drivers/media/rc/ir-lirc-codec.c +3
-0+if (val <=0 ) +return -EINVAL ; +1) val is unsigned so it would never be less than zero.
Quoted 4 lines
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.
Signature · 2 lines
Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once.
PATCH v3 1/1 rc: divide by zero bugs in s_tx_carrier()
In reply to Sean Young"carrier" comes from a get_user() in ir_lirc_ioctl(). We need to test that it's not zero before using it as a divisor. It might have been nice to test for this ir_lirc_ioctl() but the mceusb driver uses zero to disable carrier modulation.
The bug in redrat3 is a little more subtle. The ->carrier is passed to mod_freq_to_val() which uses it as a divisor.
---
v2: tried to add the check to ir_lirc_ioctl() but that doesn't work.
v3: the same as v1 except that I've added a fix for redrat3 as well.
drivers/media/rc/ene_ir.c +4 -1
@@ -881,10 +881,13 @@ static int ene_set_tx_mask(struct rc_dev *rdev, u32 tx_mask)static int ene_set_tx_carrier (struct rc_dev *rdev,u32 carrier) {struct ene_device *dev = rdev->priv; -u32 period =2000000 / carrier; +u32 period;dbg ("TX: attempt to set tx carrier to %d kHz" , carrier); +if (carrier =0 ) +return -EINVAL ; + period =2000000 / carrier;if (period && (period >ENE_CIRMOD_PRD_MAX || period <ENE_CIRMOD_PRD_MIN )) {
drivers/media/rc/nuvoton-cir.c +3 -0
@@ -517,6 +517,9 @@ static int nvt_set_tx_carrier(struct rc_dev *dev, u32 carrier)struct nvt_dev *nvt = dev->priv;u16 val; +if (carrier =0 ) +return -EINVAL ; +nvt_cir_reg_write (nvt,1 ,CIR_CP ); val =3000000 / (carrier) -1 ;nvt_cir_reg_write (nvt, val &0xff ,CIR_CC );
drivers/media/rc/redrat3.c +3 -0
@@ -890,6 +890,9 @@ static int redrat3_set_tx_carrier(struct rc_dev *rcdev, u32 carrier)struct device *dev = rr3->dev;rr3_dbg (dev,"Setting modulation frequency to %u" , carrier); +if (carrier =0 ) +return -EINVAL ; + rr3->carrier = carrier;return carrier;