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

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

[Dan Carpenter]

On Fri, Nov 26, 2010 at 08:06:35PM +0300, Vasiliy Kulikov wrote:
>  	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

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

[Jarod Wilson]

On Thu, Dec 02, 2010 at 07:51:26AM +0300, Dan Carpenter wrote:
> On Fri, Nov 26, 2010 at 08:06:35PM +0300, Vasiliy Kulikov wrote:
> >  	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.

> 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?

-- 
Jarod Wilson
jarod@redhat.com

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

[Jarod Wilson]

On Dec 2, 2010, at 10:00 AM, Jarod Wilson wrote:

> On Thu, Dec 02, 2010 at 07:51:26AM +0300, Dan Carpenter wrote:
>> On Fri, Nov 26, 2010 at 08:06:35PM +0300, Vasiliy Kulikov wrote:
>>> 	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.
> 
>> 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?

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

-- 
Jarod Wilson
jarod@wilsonet.com

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

[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 <segoon@openwall.com>
---
 Compile tested only.

 drivers/media/rc/ir-lirc-codec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 1e87ee8..a7e91e6 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -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)
-- 
1.7.0.4

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

[Jarod Wilson]

On Sun, Dec 05, 2010 at 12:05:22AM +0300, Vasiliy Kulikov wrote:
> '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 <segoon@openwall.com>

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 <jarod@redhat.com>

-- 
Jarod Wilson
jarod@redhat.com

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

[Andy Walls]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1587 bytes --]

64 bit value / 4 = 62 bit value, right?


Jarod Wilson <jarod@redhat.com> wrote:

>On Thu, Dec 02, 2010 at 07:51:26AM +0300, Dan Carpenter wrote:
>> On Fri, Nov 26, 2010 at 08:06:35PM +0300, Vasiliy Kulikov wrote:
>> >  	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.
>
>> 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?
>
>-- 
>Jarod Wilson
>jarod@redhat.com
>
>--
>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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Vasiliy Kulikov]

'n' may be bigger than MAX_INT*sizeof(int), if so checking of truncated
(int)(n/sizeof(int)) for LIRCBUF_SIZE overflow and then using nontruncated 'count'
doesn't make sense.  Also n may be up to sizeof(int)-1 bytes bigger than expected,
so check value of (n % sizeof(int)) too.

Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
---
 Compile tested only.

 drivers/media/rc/ir-lirc-codec.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
index 1e87ee8..f011c5d 100644
--- a/drivers/media/rc/ir-lirc-codec.c
+++ b/drivers/media/rc/ir-lirc-codec.c
@@ -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)
@@ -110,7 +111,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char *buf,
 		return -EINVAL;
 
 	count = n / sizeof(int);
-	if (count > LIRCBUF_SIZE || count % 2 == 0)
+	if (count > LIRCBUF_SIZE || count % 2 == 0 || n % sizeof(int) != 0)
 		return -EINVAL;
 
 	txbuf = memdup_user(buf, n);
-- 
1.7.0.4

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

[Jarod Wilson]

On Nov 26, 2010, at 12:06 PM, Vasiliy Kulikov wrote:

> 'n' may be bigger than MAX_INT*sizeof(int), if so checking of truncated
> (int)(n/sizeof(int)) for LIRCBUF_SIZE overflow and then using nontruncated 'count'
> doesn't make sense.  Also n may be up to sizeof(int)-1 bytes bigger than expected,
> so check value of (n % sizeof(int)) too.
> 
> Signed-off-by: Vasiliy Kulikov <segoon@openwall.com>
> ---
> Compile tested only.

Looks sane.

Acked-by: Jarod Wilson <jarod@redhat.com>

-- 
Jarod Wilson
jarod@wilsonet.com