Skip to contentSkip to conversation

media: rc: ir-lirc-codec: fix potential integer overflow

PATCH In review Dan Carpenter 4 participants 5 messages Dec 2, 2010 – Dec 8, 2010

Subsystems (3)
Media (V4L/DVB) Kernel janitors RC-core / lirc framework
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Review

Quoted 3 lines · Vasiliy Kulikov
(quoted diff) +1 -1
 	count = n / sizeof(int);
-	if (count > LIRCBUF_SIZE || count % 2 = 0)
+	if (count > LIRCBUF_SIZE || count % 2 = 0 || n % sizeof(int) != 0)

                                                      ^^^^^^^^^^^^^^^^^^^^

Wait, what? We just checked this a couple lines before.

The rest of the patch is right and a clever catch. It would affect x86_64 systems and not i386. This doesn't have security implications does it? You'd just catch the kmalloc() stack trace for insanely large allocations.

regards,
dan carpenter

#1 lore raw
Quoted 7 lines · Dan Carpenter

Review

Quoted 3 lines · Vasiliy Kulikov
(quoted diff) +1 -1
 	count = n / sizeof(int);
-	if (count > LIRCBUF_SIZE || count % 2 = 0)
+	if (count > LIRCBUF_SIZE || count % 2 = 0 || n % sizeof(int) != 0)

                                                      ^^^^^^^^^^^^^^^^^^^^

Wait, what? We just checked this a couple lines before.

Bah. I'd only looked at the diff, which didn't have enough context. I thought that looked familiar. Indeed, this part seems to be unnecessary.

Quoted 4 lines

The rest of the patch is right and a clever catch. It would affect x86_64 systems and not i386. This doesn't have security implications does it? You'd just catch the kmalloc() stack trace for insanely large allocations.

Even on x86_64, it looks to my (relatively untrained) eye like you'd actually be fine. n is a size_t (so, 64-bit on x86_64). count is an int (so 32-bit on x86_64). We initialize count to some 64-bit value / 4, so at most, 16 bits, which always fits just fine in the 32-bit int, no?

Signature · 2 lines
Jarod Wilson
[email protected]
#2 lore raw
Quoted 17 lines · Jarod Wilson
Quoted 7 lines · Dan Carpenter

Review

Quoted 3 lines · Vasiliy Kulikov
	count = n / sizeof(int);
(quoted diff) +1 -1
-	if (count > LIRCBUF_SIZE || count % 2 = 0)
+	if (count > LIRCBUF_SIZE || count % 2 = 0 || n % sizeof(int) != 0)

                                                     ^^^^^^^^^^^^^^^^^^^^

Wait, what? We just checked this a couple lines before.

Bah. I'd only looked at the diff, which didn't have enough context. I thought that looked familiar. Indeed, this part seems to be unnecessary.

Quoted 4 lines

The rest of the patch is right and a clever catch. It would affect x86_64 systems and not i386. This doesn't have security implications does it? You'd just catch the kmalloc() stack trace for insanely large allocations.

Quoted 4 lines

Even on x86_64, it looks to my (relatively untrained) eye like you'd actually be fine. n is a size_t (so, 64-bit on x86_64). count is an int (so 32-bit on x86_64). We initialize count to some 64-bit value / 4, so at most, 16 bits, which always fits just fine in the 32-bit int, no?

Never mind, I shouldn't be allowed near computers on too little sleep. Its been pointed out to me how incredibly incorrect and stupid what I said above is. :)

(i.e., we're not dividing the bits by 4, we're dividing a 64-bit value by 4, so you're still in 62-bit territory.)

/me sticks head back in sand

Signature · 2 lines
Jarod Wilson
[email protected]
#3 lore raw

PATCH v2 1/1 media: rc: ir-lirc-codec: fix integer overflow

In reply to Dan Carpenter

'n' may be bigger than MAX_INT*sizeof(int), if so checking of truncated (int)(n/sizeof(int)) for LIRCBUF_SIZE overflows and then using nontruncated 'count' doesn't make sense. This is not a security issue as too big 'n' is catched in kmalloc() in memdup_user() call. However, it's better to prevent WARN() in kmalloc().

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Compile tested only.

 drivers/media/rc/ir-lirc-codec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/media/rc/ir-lirc-codec.c +2 -1
index 1e87ee8..a7e91e6 100644
@@ -100,7 +100,8 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char *buf,
 	struct lirc_codec *lirc;
 	struct rc_dev *dev;
 	int *txbuf; /* buffer with values to transmit */
-	int ret = 0, count;
+	int ret = 0;
+	size_t count;
 
 	lirc = lirc_get_pdata(file);
 	if (!lirc)
Signature · 1 line
1.7.0.4
Acked-by on v2 1/1
#4 lore raw

Re: [PATCH v2] media: rc: ir-lirc-codec: fix integer overflow

Quoted 6 lines · Vasiliy Kulikov

'n' may be bigger than MAX_INT*sizeof(int), if so checking of truncated (int)(n/sizeof(int)) for LIRCBUF_SIZE overflows and then using nontruncated 'count' doesn't make sense. This is not a security issue as too big 'n' is catched in kmalloc() in memdup_user() call. However, it's better to prevent WARN() in kmalloc().

Signed-off-by Vasiliy Kulikov <[email protected]>

Now that I have my head out of my arse wrt the actual issue here, the redundancy issue from v1 is resolved, and I've managed a full night's sleep... ;)

Acked-by Jarod Wilson <[email protected]>
Signature · 2 lines
Jarod Wilson
[email protected]