Skip to contentSkip to conversation

av7110: make array offset unsigned

PATCH In review Dan Carpenter 6 participants 11 messages Jan 6, 2011 – Jan 12, 2011

Subsystems (2)
Media (V4L/DVB) Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 av7110: make array offset unsigned

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 <[email protected]>

---
Compile tested.

include/linux/dvb/ca.h +1 -1
index c18537f..647015e 100644
@@ -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 */
#1 lore raw
Quoted 10 lines · 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;
Quoted 3 lines

Let's just make it unsigned.

Signed-off-by Dan Carpenter <[email protected]>

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

Regards,
Andreas

author
#2 lore raw

PATCH v2 1/1 av7110: check for negative array offset

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 <[email protected]>

---
V2: change the check instead of making num and unsigned int

drivers/media/dvb/ttpci/av7110_ca.c +1 -1
index 122c728..923a8e2 100644
@@ -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) ?
#3 lore raw

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

Hi,

Quoted 21 lines · 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 <[email protected]>

---
V2: change the check instead of making num and unsigned int

drivers/media/dvb/ttpci/av7110_ca.c +1 -2
index 122c728..923a8e2 100644
@@ -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) ?
--
Quoted 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
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

Signature · 5 lines
----------------------------------------------------------------
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/
----------------------------------------------------------------
author
#4 lore raw

PATCH v3 1/1 av7110: check for negative array offset

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 <[email protected]>

---
V2: change the check instead of making num and unsigned int
V3: white space changes

drivers/media/dvb/ttpci/av7110_ca.c +1 -1
index 122c728..923a8e2 100644
@@ -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) ?
#5 lore raw

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

Am 07.01.2011 20:41, schrieb Dan Carpenter:

Quoted 13 lines

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 <[email protected]>

---
V2: change the check instead of making num and unsigned int
V3: white space changes

drivers/media/dvb/ttpci/av7110_ca.c +0 -0
index 122c728..923a8e2 100644

drivers/media/dvb/ttpci/av7110_ca.c static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg) · line 281

Quoted 6 lines
drivers/media/dvb/ttpci/av7110_ca.c +1 -1
@@ -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

Quoted 9 lines
drivers/media/dvb/ttpci/av7110_ca.c +0 -1
 		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

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

In reply to Dan Carpenter
Quoted 8 lines · walter harms
Quoted 8 lines
drivers/media/dvb/ttpci/av7110_ca.c +0 -0
index 122c728..923a8e2 100644
@@ -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;
Quoted 6 lines

drivers/media/dvb/ttpci/av7110_ca.c static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg) · line 281

Quoted 3 lines
drivers/media/dvb/ttpci/av7110_ca.c +1 -1
-		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw

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

In reply to Dan Carpenter

Am 12.01.2011 14:30, schrieb Jorge Merlino:

Quoted 9 lines · walter harms
Quoted 8 lines
Quoted 8 lines
drivers/media/dvb/ttpci/av7110_ca.c +0 -0
index 122c728..923a8e2 100644
@@ -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;
Quoted 8 lines
Quoted 6 lines

drivers/media/dvb/ttpci/av7110_ca.c static int dvb_ca_ioctl(struct file *file, unsigned int cmd, void *parg) · line 281

Quoted 3 lines
drivers/media/dvb/ttpci/av7110_ca.c +1 -1
-		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#8 lore raw
In reply to Andreas Oberritter
Quoted 3 lines · Andreas Oberritter

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

#9 lore raw
Quoted 2 lines · Dan Carpenter

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.

Signature · 1 line
Vasiliy
#10 lore raw
In reply to Dan Carpenter
Quoted 6 lines · Dan Carpenter
Quoted 3 lines · Andreas Oberritter

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.

Quoted 3 lines

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