Message ID | 20120909203142.GA12296@elgon.mountain (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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
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.
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
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 --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:
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