* [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
@ 2012-09-09 20:31 Dan Carpenter
2012-09-09 21:01 ` walter harms
2012-09-09 22:26 ` Sean Young
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2012-09-09 20:31 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Paul Gortmaker, Sean Young, David Härdeman, Ben Hutchings,
linux-media, kernel-janitors
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:
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
2012-09-09 20:31 [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier() Dan Carpenter
@ 2012-09-09 21:01 ` walter harms
2012-09-09 22:00 ` Ben Hutchings
2012-09-09 22:26 ` Sean Young
1 sibling, 1 reply; 6+ messages in thread
From: walter harms @ 2012-09-09 21:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Paul Gortmaker, Sean Young,
David Härdeman, Ben Hutchings, linux-media, kernel-janitors
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:
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
2012-09-09 21:01 ` walter harms
@ 2012-09-09 22:00 ` Ben Hutchings
0 siblings, 0 replies; 6+ messages in thread
From: Ben Hutchings @ 2012-09-09 22:00 UTC (permalink / raw)
To: wharms
Cc: Dan Carpenter, Mauro Carvalho Chehab, Paul Gortmaker, Sean Young,
David Härdeman, linux-media, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1379 bytes --]
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.
--
Ben Hutchings
Time is nature's way of making sure that everything doesn't happen at once.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
2012-09-09 20:31 [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier() Dan Carpenter
2012-09-09 21:01 ` walter harms
@ 2012-09-09 22:26 ` Sean Young
2012-09-09 22:38 ` Ben Hutchings
2012-09-11 11:11 ` [patch v3] [media] rc: divide by zero bugs " Dan Carpenter
1 sibling, 2 replies; 6+ messages in thread
From: Sean Young @ 2012-09-09 22:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mauro Carvalho Chehab, Paul Gortmaker, David Härdeman,
Ben Hutchings, linux-media, kernel-janitors
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()
2012-09-09 22:26 ` Sean Young
@ 2012-09-09 22:38 ` Ben Hutchings
2012-09-11 11:11 ` [patch v3] [media] rc: divide by zero bugs " Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Ben Hutchings @ 2012-09-09 22:38 UTC (permalink / raw)
To: Sean Young
Cc: Dan Carpenter, Mauro Carvalho Chehab, Paul Gortmaker,
David Härdeman, linux-media, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]
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.
--
Ben Hutchings
Time is nature's way of making sure that everything doesn't happen at once.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch v3] [media] rc: divide by zero bugs in s_tx_carrier()
2012-09-09 22:26 ` Sean Young
2012-09-09 22:38 ` Ben Hutchings
@ 2012-09-11 11:11 ` Dan Carpenter
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2012-09-11 11:11 UTC (permalink / raw)
To: Maxim Levitsky; +Cc: Mauro Carvalho Chehab, linux-media, kernel-janitors
"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.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
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.
diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c
index 647dd95..d05ac15 100644
--- a/drivers/media/rc/ene_ir.c
+++ b/drivers/media/rc/ene_ir.c
@@ -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)) {
diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 699eef3..2ea913a 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -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);
diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
index 2878b0e..bf8bc74 100644
--- a/drivers/media/rc/redrat3.c
+++ b/drivers/media/rc/redrat3.c
@@ -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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-11 11:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-09 20:31 [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier() Dan Carpenter
2012-09-09 21:01 ` walter harms
2012-09-09 22:00 ` Ben Hutchings
2012-09-09 22:26 ` Sean Young
2012-09-09 22:38 ` Ben Hutchings
2012-09-11 11:11 ` [patch v3] [media] rc: divide by zero bugs " Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).