'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 <[email protected]>
---
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
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 <[email protected]>
> ---
> Compile tested only.
Looks sane.
Acked-by: Jarod Wilson <[email protected]>
--
Jarod Wilson
[email protected]
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
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
[email protected]
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
[email protected]
64 bit value / 4 = 62 bit value, right?
Jarod Wilson <[email protected]> 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
>[email protected]
>
>--
>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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
'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(-)
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
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 <[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]>
--
Jarod Wilson
[email protected]