[patch] [media] av7110: make array offset unsigned

[patch] [media] av7110: make array offset unsigned

[Dan Carpenter]

In the CA_GET_SLOT_INFO ioctl, we only check whether "num" is too large,
but we don't check if it's negative.

drivers/media/dvb/ttpci/av7110_ca.c
   278		ca_slot_info_t *info=(ca_slot_info_t *)parg;
   279
   280		if (info->num > 1)
   281			return -EINVAL;
   282		av7110->ci_slot[info->num].num = info->num;

Let's just make it unsigned.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Compile tested.

diff --git a/include/linux/dvb/ca.h b/include/linux/dvb/ca.h
index c18537f..647015e 100644
--- a/include/linux/dvb/ca.h
+++ b/include/linux/dvb/ca.h
@@ -27,7 +27,7 @@
 /* slot interface types and info */
 
 typedef struct ca_slot_info {
-	int num;               /* slot number */
+	unsigned int num;      /* slot number */
 
 	int type;              /* CA interface this slot supports */
 #define CA_CI            1     /* CI high level interface */

Re: [patch] [media] av7110: make array offset unsigned

[Andreas Oberritter]

On 01/06/2011 08:41 PM, Dan Carpenter wrote:
> In the CA_GET_SLOT_INFO ioctl, we only check whether "num" is too large,
> but we don't check if it's negative.
> 
> drivers/media/dvb/ttpci/av7110_ca.c
>    278		ca_slot_info_t *info=(ca_slot_info_t *)parg;
>    279
>    280		if (info->num > 1)
>    281			return -EINVAL;
>    282		av7110->ci_slot[info->num].num = info->num;
> 
> Let's just make it unsigned.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Nack. You're changing an interface to userspace. Please add a check to
av7110_ca.c instead.

Regards,
Andreas

[patch v2] [media] av7110: check for negative array offset

[Dan Carpenter]

info->num comes from the user.  It's type int.  If the user passes
in a negative value that would cause memory corruption.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2: change the check instead of making num and unsigned int

diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
index 122c728..923a8e2 100644
--- a/drivers/media/dvb/ttpci/av7110_ca.c
+++ b/drivers/media/dvb/ttpci/av7110_ca.c
@@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
 	{
 		ca_slot_info_t *info=(ca_slot_info_t *)parg;
 
-		if (info->num > 1)
+		if ((unsigned)info->num > 1)
 			return -EINVAL;
 		av7110->ci_slot[info->num].num = info->num;
 		av7110->ci_slot[info->num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?

Re: [patch] [media] av7110: make array offset unsigned

[Dan Carpenter]

On Fri, Jan 07, 2011 at 01:44:15PM +0100, Andreas Oberritter wrote:
> Nack. You're changing an interface to userspace. Please add a check to
> av7110_ca.c instead.
> 

Ok.  I've done that and resent the patch.

But just for my own understanding, why is it wrong to change an int to
an unsigned int in the userspace API?  Who would notice?  (I'm still
quite a newbie at system programming).

regards,
dan carpenter

Re: [patch] [media] av7110: make array offset unsigned

[Vasiliy Kulikov]

On Fri, Jan 07, 2011 at 16:51 +0300, Dan Carpenter wrote:
> But just for my own understanding, why is it wrong to change an int to
> an unsigned int in the userspace API?  Who would notice?

E.g. the same check in userspace (var < 0).  If var has changed the sign
then the result would differ.

-- 
Vasiliy

Re: [patch] [media] av7110: make array offset unsigned

[Andreas Oberritter]

On 01/07/2011 02:51 PM, Dan Carpenter wrote:
> On Fri, Jan 07, 2011 at 01:44:15PM +0100, Andreas Oberritter wrote:
>> Nack. You're changing an interface to userspace. Please add a check to
>> av7110_ca.c instead.
>>
> 
> Ok.  I've done that and resent the patch.

Thanks. I'm OK with the patch, but I'll leave it to the maintainer of
av7110 to decide whether he likes the cast or prefers an additional
signed compare. I added him to CC.

> But just for my own understanding, why is it wrong to change an int to
> an unsigned int in the userspace API?  Who would notice?  (I'm still
> quite a newbie at system programming).

It would generate compiler warnings in userspace for programs checking
for values < 0 or assigning negative values (for whatever reason).

Regards,
Andreas

Re: [patch v2] [media] av7110: check for negative array offset

[Oliver Endriss]

Hi,

On Friday 07 January 2011 14:46:51 Dan Carpenter wrote:
> info->num comes from the user.  It's type int.  If the user passes
> in a negative value that would cause memory corruption.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2: change the check instead of making num and unsigned int
> 
> diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
> index 122c728..923a8e2 100644
> --- a/drivers/media/dvb/ttpci/av7110_ca.c
> +++ b/drivers/media/dvb/ttpci/av7110_ca.c
> @@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
>  	{
>  		ca_slot_info_t *info=(ca_slot_info_t *)parg;
>  
> -		if (info->num > 1)
> +		if ((unsigned)info->num > 1)
>  			return -EINVAL;
>  		av7110->ci_slot[info->num].num = info->num;
>  		av7110->ci_slot[info->num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?
> --
> 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

Imho casts are a last resort and should be avoided, if there is a better
way to do it. The obvious fix is

  if (info->num < 0 || info->num > 1)
        return -EINVAL;
   
CU
Oliver

-- 
----------------------------------------------------------------
VDR Remote Plugin 0.4.0: http://www.escape-edv.de/endriss/vdr/
4 MByte Mod: http://www.escape-edv.de/endriss/dvb-mem-mod/
Full-TS Mod: http://www.escape-edv.de/endriss/dvb-full-ts-mod/
----------------------------------------------------------------

[patch v3] [media] av7110: check for negative array offset

[Dan Carpenter]

info->num comes from the user.  It's type int.  If the user passes
in a negative value that would cause memory corruption.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2: change the check instead of making num and unsigned int
V3: white space changes

diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
index 122c728..923a8e2 100644
--- a/drivers/media/dvb/ttpci/av7110_ca.c
+++ b/drivers/media/dvb/ttpci/av7110_ca.c
@@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
 	{
 		ca_slot_info_t *info=(ca_slot_info_t *)parg;
 
-		if (info->num > 1)
+		if (info->num < 0 || info->num > 1)
 			return -EINVAL;
 		av7110->ci_slot[info->num].num = info->num;
 		av7110->ci_slot[info->num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?

Re: [patch v3] [media] av7110: check for negative array offset

[walter harms]

Am 07.01.2011 20:41, schrieb Dan Carpenter:
> info->num comes from the user.  It's type int.  If the user passes
> in a negative value that would cause memory corruption.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> V2: change the check instead of making num and unsigned int
> V3: white space changes
> 
> diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
> index 122c728..923a8e2 100644
> --- a/drivers/media/dvb/ttpci/av7110_ca.c
> +++ b/drivers/media/dvb/ttpci/av7110_ca.c
> @@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
>  	{
>  		ca_slot_info_t *info=(ca_slot_info_t *)parg;
>  
> -		if (info->num > 1)
> +		if (info->num < 0 || info->num > 1)
>  			return -EINVAL;

maybe i miss the point but anything else than 0 will cause -EINVAL ?
Is that intended ?

re,
 wh


>  		av7110->ci_slot[info->num].num = info->num;
>  		av7110->ci_slot[info->num].type = FW_CI_LL_SUPPORT(av7110->arm_app) ?
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch v3] [media] av7110: check for negative array offset

[Jorge Merlino]

El 12/01/11 10:44, walter harms escribió:
>> diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
>> index 122c728..923a8e2 100644
>> --- a/drivers/media/dvb/ttpci/av7110_ca.c
>> +++ b/drivers/media/dvb/ttpci/av7110_ca.c
>> @@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
>>  	{
>>  		ca_slot_info_t *info=(ca_slot_info_t *)parg;
>>  
>> -		if (info->num > 1)
>> +		if (info->num < 0 || info->num > 1)
>>  			return -EINVAL;
> 
> maybe i miss the point but anything else than 0 will cause -EINVAL ?
> Is that intended ?

Anything else than 0 or 1.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v3] [media] av7110: check for negative array offset

[walter harms]

Am 12.01.2011 14:30, schrieb Jorge Merlino:
> El 12/01/11 10:44, walter harms escribió:
>>> diff --git a/drivers/media/dvb/ttpci/av7110_ca.c b/drivers/media/dvb/ttpci/av7110_ca.c
>>> index 122c728..923a8e2 100644
>>> --- a/drivers/media/dvb/ttpci/av7110_ca.c
>>> +++ b/drivers/media/dvb/ttpci/av7110_ca.c
>>> @@ -277,7 +277,7 @@ static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg)
>>>  	{
>>>  		ca_slot_info_t *info=(ca_slot_info_t *)parg;
>>>  
>>> -		if (info->num > 1)
>>> +		if (info->num < 0 || info->num > 1)
>>>  			return -EINVAL;
>>
>> maybe i miss the point but anything else than 0 will cause -EINVAL ?
>> Is that intended ?
> 
> Anything else than 0 or 1.


mmmh, yes my bad,
ntl can the "1" be replaced with array_size(av7110->ci_slot)-1 ?

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