media/IR/imon: array overflow

-next Superseded Dan Carpenter 3 participants 6 messages May 4, 2010 – May 7, 2010 Media (V4L/DVB)

Share & tools

Dan Carpenter

author

2010-05-04 12:20 UTC #0 lore raw

This is a Smatch thing. I'm not sure what's intended.

drivers/media/IR/imon.c +1211 imon_panel_key_lookup(10) error: buffer overflow 'imon_panel_key_table' 21 <= 21
1207 for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++)
1208 if (imon_panel_key_table[i].hw_code == (code | 0xffee))
1209 break;

After the for loop i can be 0 to 21.

  1210
  1211          keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;

IMON_KEY_RELEASE_OFFSET is 1000 so it doesn't affect anything.

regards,
dan carpenter

Jarod Wilson

2010-05-04 14:03 UTC #1 lore raw

PATCH 1/1 IR/imon: remove dead IMON_KEY_RELEASE_OFFSET

This hack was used when the imon driver was using internal key lookup routines, but became dead weight when the driver was converted to use ir-core's key lookup routines. These bits simply didn't get removed, drop 'em now.

Pointed out by Dan Carpenter.

Signed-off-by Jarod Wilson <[email protected]>

---
 drivers/media/IR/imon.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

drivers/media/IR/imon.c +1 -2

index 27743eb..bce8ef8 100644
@@ -55,7 +55,6 @@
 #define BIT_DURATION	250	/* each bit received is 250us */
 
 #define IMON_CLOCK_ENABLE_PACKETS	2
-#define IMON_KEY_RELEASE_OFFSET		1000
 
 /*** P R O T O T Y P E S ***/

@@ -1205,7 +1204,7 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
 			break;
 
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+	keycode = imon_panel_key_table[i].keycode;
 
 	return keycode;
 }

Signature · 2 lines

Jarod Wilson
[email protected]

Dan Carpenter

author

2010-05-04 16:06 UTC #2 lore raw

Re: [PATCH] IR/imon: remove dead IMON_KEY_RELEASE_OFFSET

drivers/media/IR/imon.c static u32 imon_panel_key_lookup(u64 hw_code) · line 1210

Quoted 9 lines · Jarod Wilson

drivers/media/IR/imon.c +1 -1

@@ -1205,7 +1204,7 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
 			break;
 
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+	keycode = imon_panel_key_table[i].keycode;
 
 	return keycode;
 }

There is still potentially a problem here because if we don't hit the break statement, then we're one past the end of the array.

regards,
dan carpenter

Jarod Wilson

2010-05-04 19:17 UTC #3 lore raw

PATCH v2 1/1 IR/imon: remove dead IMON_KEY_RELEASE_OFFSET

Quoted 5 lines · Dan Carpenter

Quoted 4 lines · Jarod Wilson

(quoted diff) +0 -0

@@ -1205,7 +1204,7 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
 			break;

Quoted 8 lines

Review static u32 imon_panel_key_lookup(u64 hw_code) · line 1210
Quoted 5 lines
(quoted diff) +1 -1
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+	keycode = imon_panel_key_table[i].keycode;
 
 	return keycode;
 }
There is still potentially a problem here because if we don't hit the break statement, then we're one past the end of the array.

D'oh. Okay, here's v2, should fix that buglet too.

Pointed out by Dan Carpenter.

v2: fix possible attempt to access beyond end of key table array, also pointed out by Dan.

---
 drivers/media/IR/imon.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/media/IR/imon.c +6 -6

index 27743eb..efe219a 100644
@@ -55,7 +55,6 @@
 #define BIT_DURATION	250	/* each bit received is 250us */
 
 #define IMON_CLOCK_ENABLE_PACKETS	2
-#define IMON_KEY_RELEASE_OFFSET		1000
 
 /*** P R O T O T Y P E S ***/

@@ -1199,13 +1198,14 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 {
 	int i;
 	u64 code = be64_to_cpu(hw_code);
-	u32 keycode;
+	u32 keycode = KEY_RESERVED;
 
-	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++)
-		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
+	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++) {
+		if (imon_panel_key_table[i].hw_code == (code | 0xffee)) {
+			keycode = imon_panel_key_table[i].keycode;
 			break;
-
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+		}
+	}
 
 	return keycode;
 }

Signature · 2 lines

Jarod Wilson
[email protected]

Mauro Carvalho Chehab

2010-05-07 11:52 UTC #4 lore raw

Re: [PATCH v2] IR/imon: remove dead IMON_KEY_RELEASE_OFFSET

Quoted 21 lines · Jarod Wilson

Quoted 5 lines · Dan Carpenter
Quoted 4 lines · Jarod Wilson
(quoted diff) +0 -0
@@ -1205,7 +1204,7 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
 			break;
 
Quoted 7 lines
drivers/media/IR/imon.c static u32 imon_panel_key_lookup(u64 hw_code) · line 1211
Quoted 5 lines
drivers/media/IR/imon.c +1 -1
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+	keycode = imon_panel_key_table[i].keycode;
 
 	return keycode;
 }
There is still potentially a problem here because if we don't hit the break statement, then we're one past the end of the array.
D'oh. Okay, here's v2, should fix that buglet too.
This hack was used when the imon driver was using internal key lookup routines, but became dead weight when the driver was converted to use ir-core's key lookup routines. These bits simply didn't get removed, drop 'em now.

Quoted 4 lines

Pointed out by Dan Carpenter.
v2: fix possible attempt to access beyond end of key table array, also pointed out by Dan.

-ENOSOB

Please, add your SOB here ;)

Quoted 38 lines

---
 drivers/media/IR/imon.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/media/IR/imon.c +6 -6

index 27743eb..efe219a 100644
@@ -55,7 +55,6 @@
 #define BIT_DURATION	250	/* each bit received is 250us */
 
 #define IMON_CLOCK_ENABLE_PACKETS	2
-#define IMON_KEY_RELEASE_OFFSET		1000
 
 /*** P R O T O T Y P E S ***/

@@ -1199,13 +1198,14 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 {
 	int i;
 	u64 code = be64_to_cpu(hw_code);
-	u32 keycode;
+	u32 keycode = KEY_RESERVED;
 
-	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++)
-		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
+	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++) {
+		if (imon_panel_key_table[i].hw_code == (code | 0xffee)) {
+			keycode = imon_panel_key_table[i].keycode;
 			break;
-
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+		}
+	}
 
 	return keycode;
 }

Signature · 2 lines

Cheers,
Mauro

Jarod Wilson

2010-05-07 13:13 UTC #5 lore raw

Re: [PATCH v2] IR/imon: remove dead IMON_KEY_RELEASE_OFFSET

Quoted 22 lines · Mauro Carvalho Chehab

Quoted 21 lines · Jarod Wilson
Quoted 5 lines · Dan Carpenter
Quoted 4 lines · Jarod Wilson
(quoted diff) +0 -0
@@ -1205,7 +1204,7 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
 			break;
 
Quoted 7 lines
drivers/media/IR/imon.c static u32 imon_panel_key_lookup(u64 hw_code) · line 1211
Quoted 5 lines
drivers/media/IR/imon.c +1 -1
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+	keycode = imon_panel_key_table[i].keycode;
 
 	return keycode;
 }
There is still potentially a problem here because if we don't hit the break statement, then we're one past the end of the array.
D'oh. Okay, here's v2, should fix that buglet too.
This hack was used when the imon driver was using internal key lookup routines, but became dead weight when the driver was converted to use ir-core's key lookup routines. These bits simply didn't get removed, drop 'em now.

Quoted 8 lines

Quoted 4 lines
Pointed out by Dan Carpenter.
v2: fix possible attempt to access beyond end of key table array, also pointed out by Dan.
-ENOSOB
Please, add your SOB here ;)

D'oh.

Signed-off-by Jarod Wilson <[email protected]>

Quoted 43 lines

Quoted 37 lines

---
 drivers/media/IR/imon.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

drivers/media/IR/imon.c +6 -6

index 27743eb..efe219a 100644
@@ -55,7 +55,6 @@
 #define BIT_DURATION	250	/* each bit received is 250us */
 
 #define IMON_CLOCK_ENABLE_PACKETS	2
-#define IMON_KEY_RELEASE_OFFSET		1000
 
 /*** P R O T O T Y P E S ***/

@@ -1199,13 +1198,14 @@ static u32 imon_panel_key_lookup(u64 hw_code)
 {
 	int i;
 	u64 code = be64_to_cpu(hw_code);
-	u32 keycode;
+	u32 keycode = KEY_RESERVED;
 
-	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++)
-		if (imon_panel_key_table[i].hw_code == (code | 0xffee))
+	for (i = 0; i < ARRAY_SIZE(imon_panel_key_table); i++) {
+		if (imon_panel_key_table[i].hw_code == (code | 0xffee)) {
+			keycode = imon_panel_key_table[i].keycode;
 			break;
-
-	keycode = imon_panel_key_table[i % IMON_KEY_RELEASE_OFFSET].keycode;
+		}
+	}
 
 	return keycode;
 }

Signature · 2 lines

Cheers,
Mauro

Signature · 2 lines

Jarod Wilson
[email protected]