* [PATCH] comedi: integer overflow in do_insnlist_ioctl()
@ 2011-11-23 0:49 Xi Wang
2011-11-23 6:13 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Xi Wang @ 2011-11-23 0:49 UTC (permalink / raw)
To: linux-kernel
Cc: Ian Abbott, Mori Hess, Greg Kroah-Hartman, Mark Pearson,
Lucas De Marchi, Greg Dietsche, Franky Lin, devel, security
There is a potential integer overflow in do_insnlist_ioctl() if userspace passes in a large insnlist.n_insns. The call to kmalloc() would allocate a small buffer, which would result in a memory corruption.
Reported-by: Haogang Chen <haogangchen@gmail.com>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
drivers/staging/comedi/comedi_fops.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 21d8c1c..66bb49d 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -650,6 +650,7 @@ static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn,
* data (for reads)
*/
/* arbitrary limits */
+#define MAX_INSNS 256
#define MAX_SAMPLES 256
static int do_insnlist_ioctl(struct comedi_device *dev,
struct comedi_insnlist __user *arg, void *file)
@@ -663,6 +664,12 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
if (copy_from_user(&insnlist, arg, sizeof(struct comedi_insnlist)))
return -EFAULT;
+ if (insnlist.n_insns > MAX_INSNS) {
+ DPRINTK("invalid number of instructions\n");
+ ret = -EINVAL;
+ goto error;
+ }
+
data = kmalloc(sizeof(unsigned int) * MAX_SAMPLES, GFP_KERNEL);
if (!data) {
DPRINTK("kmalloc failed\n");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 0:49 [PATCH] comedi: integer overflow in do_insnlist_ioctl() Xi Wang
@ 2011-11-23 6:13 ` Dan Carpenter
2011-11-23 13:59 ` Xi Wang
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2011-11-23 6:13 UTC (permalink / raw)
To: Xi Wang
Cc: linux-kernel, devel, Mori Hess, security, Lucas De Marchi,
Greg Kroah-Hartman, Ian Abbott, Franky Lin, Greg Dietsche,
Mark Pearson
[-- Attachment #1: Type: text/plain, Size: 142 bytes --]
I sent a patch for this already.
http://driverdev.linuxdriverproject.org/pipermail/devel/2011-November/022469.html
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 6:13 ` Dan Carpenter
@ 2011-11-23 13:59 ` Xi Wang
2011-11-23 14:50 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Xi Wang @ 2011-11-23 13:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-kernel, devel, Mori Hess, security, Lucas De Marchi,
Greg Kroah-Hartman, Ian Abbott, Franky Lin, Greg Dietsche,
Mark Pearson
Thanks for the pointer. However you cannot do the overflow check using
if (sizeof(struct comedi_insn) * insnlist.n_insns < insnlist.n_insns)
Let's assume 32-bit system, sizeof(struct comedi_insn) = 32, and
insnlist.n_insns = 0x7fffffff.
Note that 32 * 0x7fffffff = 0xffffffe0 overflows but bypasses your check.
- xi
On Wed, Nov 23, 2011 at 1:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> I sent a patch for this already.
>
> http://driverdev.linuxdriverproject.org/pipermail/devel/2011-November/022469.html
>
> regards,
> dan carpenter
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 13:59 ` Xi Wang
@ 2011-11-23 14:50 ` Dan Carpenter
2011-11-23 16:06 ` Ian Abbott
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2011-11-23 14:50 UTC (permalink / raw)
To: Xi Wang
Cc: linux-kernel, devel, Mori Hess, security, Lucas De Marchi,
Greg Kroah-Hartman, Ian Abbott, Franky Lin, Greg Dietsche,
Mark Pearson
[-- Attachment #1: Type: text/plain, Size: 613 bytes --]
On Wed, Nov 23, 2011 at 08:59:52AM -0500, Xi Wang wrote:
> Thanks for the pointer. However you cannot do the overflow check using
>
> if (sizeof(struct comedi_insn) * insnlist.n_insns < insnlist.n_insns)
>
> Let's assume 32-bit system, sizeof(struct comedi_insn) = 32, and
> insnlist.n_insns = 0x7fffffff.
>
> Note that 32 * 0x7fffffff = 0xffffffe0 overflows but bypasses your check.
>
Argh... You're right, my check is wrong. What I like about my patch
though is that it doesn't introduce an arbitrary limit. Could you
redo your check without the MAX_INSNS?
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 14:50 ` Dan Carpenter
@ 2011-11-23 16:06 ` Ian Abbott
2011-11-23 16:53 ` [PATCH v2] " Xi Wang
2011-11-23 21:41 ` [PATCH] " Lars-Peter Clausen
0 siblings, 2 replies; 14+ messages in thread
From: Ian Abbott @ 2011-11-23 16:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Xi Wang, linux-kernel, devel, Mori Hess, security,
Lucas De Marchi, Greg Kroah-Hartman, Ian Abbott, Franky Lin,
Greg Dietsche, Mark Pearson
On 2011-11-23 14:50, Dan Carpenter wrote:
> On Wed, Nov 23, 2011 at 08:59:52AM -0500, Xi Wang wrote:
>> Thanks for the pointer. However you cannot do the overflow check using
>>
>> if (sizeof(struct comedi_insn) * insnlist.n_insns < insnlist.n_insns)
>>
>> Let's assume 32-bit system, sizeof(struct comedi_insn) = 32, and
>> insnlist.n_insns = 0x7fffffff.
>>
>> Note that 32 * 0x7fffffff = 0xffffffe0 overflows but bypasses your check.
>>
>
> Argh... You're right, my check is wrong. What I like about my patch
> though is that it doesn't introduce an arbitrary limit. Could you
> redo your check without the MAX_INSNS?
Could use something like:
if (insnlist.n_insns <= ULONG_MAX / sizeof(struct comedi_insn))
insns =
kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns,
GFP_KERNEL);
if (!insns)
...
(note that insns is initialized to NULL).
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 16:06 ` Ian Abbott
@ 2011-11-23 16:53 ` Xi Wang
2011-11-23 21:41 ` [PATCH] " Lars-Peter Clausen
1 sibling, 0 replies; 14+ messages in thread
From: Xi Wang @ 2011-11-23 16:53 UTC (permalink / raw)
To: Ian Abbott
Cc: Dan Carpenter, linux-kernel, devel, Mori Hess, security,
Lucas De Marchi, Greg Kroah-Hartman, Ian Abbott, Franky Lin,
Greg Dietsche, Mark Pearson
There is a potential integer overflow in do_insnlist_ioctl() if
userspace passes in a large insnlist.n_insns. The call to kmalloc()
would allocate a small buffer, leading to a memory corruption.
Reported-by: Haogang Chen <haogangchen@gmail.com>
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Suggested-by: Ian Abbott <abbotti@mev.co.uk>
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
drivers/staging/comedi/comedi_fops.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
index 21d8c1c..df86a9e 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -670,8 +670,9 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
goto error;
}
- insns =
- kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns, GFP_KERNEL);
+ if (insnlist.n_insns <= ULONG_MAX / sizeof(struct comedi_insn))
+ insns = kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns,
+ GFP_KERNEL);
if (!insns) {
DPRINTK("kmalloc failed\n");
ret = -ENOMEM;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 16:06 ` Ian Abbott
2011-11-23 16:53 ` [PATCH v2] " Xi Wang
@ 2011-11-23 21:41 ` Lars-Peter Clausen
2011-11-23 21:51 ` Dan Carpenter
1 sibling, 1 reply; 14+ messages in thread
From: Lars-Peter Clausen @ 2011-11-23 21:41 UTC (permalink / raw)
To: Ian Abbott
Cc: Dan Carpenter, Xi Wang, linux-kernel, devel, Mori Hess, security,
Lucas De Marchi, Greg Kroah-Hartman, Ian Abbott, Franky Lin,
Greg Dietsche, Mark Pearson
On 11/23/2011 05:06 PM, Ian Abbott wrote:
> On 2011-11-23 14:50, Dan Carpenter wrote:
>> On Wed, Nov 23, 2011 at 08:59:52AM -0500, Xi Wang wrote:
>>> Thanks for the pointer. However you cannot do the overflow check using
>>>
>>> if (sizeof(struct comedi_insn) * insnlist.n_insns <
>>> insnlist.n_insns)
>>>
>>> Let's assume 32-bit system, sizeof(struct comedi_insn) = 32, and
>>> insnlist.n_insns = 0x7fffffff.
>>>
>>> Note that 32 * 0x7fffffff = 0xffffffe0 overflows but bypasses your
>>> check.
>>>
>>
>> Argh... You're right, my check is wrong. What I like about my patch
>> though is that it doesn't introduce an arbitrary limit. Could you
>> redo your check without the MAX_INSNS?
>
> Could use something like:
>
> if (insnlist.n_insns <= ULONG_MAX / sizeof(struct comedi_insn))
> insns =
> kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns,
> GFP_KERNEL);
> if (!insns)
> ...
>
> (note that insns is initialized to NULL).
>
Just use kcalloc, it will do the right thing for you.
- Lars
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 21:41 ` [PATCH] " Lars-Peter Clausen
@ 2011-11-23 21:51 ` Dan Carpenter
2011-11-24 19:07 ` Xi Wang
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2011-11-23 21:51 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Ian Abbott, Xi Wang, linux-kernel, devel, Mori Hess, security,
Lucas De Marchi, Greg Kroah-Hartman, Ian Abbott, Franky Lin,
Greg Dietsche, Mark Pearson
[-- Attachment #1: Type: text/plain, Size: 657 bytes --]
On Wed, Nov 23, 2011 at 10:41:07PM +0100, Lars-Peter Clausen wrote:
> > if (insnlist.n_insns <= ULONG_MAX / sizeof(struct comedi_insn))
> > insns =
> > kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns,
> > GFP_KERNEL);
> > if (!insns)
> > ...
> >
> > (note that insns is initialized to NULL).
> >
>
> Just use kcalloc, it will do the right thing for you.
>
I think the reason why I didn't do that in my original patch is that
kcalloc() has a memset(..., 0, ...) in it so it's a slow down. But
this isn't performance critical code so that would work.
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-23 21:51 ` Dan Carpenter
@ 2011-11-24 19:07 ` Xi Wang
2011-11-25 7:25 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Xi Wang @ 2011-11-24 19:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: Lars-Peter Clausen, Ian Abbott, linux-kernel, devel, Mori Hess,
security, Lucas De Marchi, Greg Kroah-Hartman, Ian Abbott,
Franky Lin, Greg Dietsche, Mark Pearson
Using kcalloc looks good to me. Do you want to redo the patch in that way?
- xi
On Nov 23, 2011, at 4:51 PM, Dan Carpenter wrote:
>
> I think the reason why I didn't do that in my original patch is that
> kcalloc() has a memset(..., 0, ...) in it so it's a slow down. But
> this isn't performance critical code so that would work.
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: integer overflow in do_insnlist_ioctl()
2011-11-24 19:07 ` Xi Wang
@ 2011-11-25 7:25 ` Dan Carpenter
2011-11-25 21:46 ` [PATCH v3] " Xi Wang
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2011-11-25 7:25 UTC (permalink / raw)
To: Xi Wang
Cc: devel, Mori Hess, security, Lars-Peter Clausen, Ian Abbott,
Lucas De Marchi, Greg Kroah-Hartman, linux-kernel, Ian Abbott,
Franky Lin, Greg Dietsche, Mark Pearson
[-- Attachment #1: Type: text/plain, Size: 234 bytes --]
On Thu, Nov 24, 2011 at 02:07:49PM -0500, Xi Wang wrote:
> Using kcalloc looks good to me. Do you want to redo the patch in that way?
>
It's your choice. The other fix you wrote is valid as well.
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] comedi: integer overflow in do_insnlist_ioctl()
2011-11-25 7:25 ` Dan Carpenter
@ 2011-11-25 21:46 ` Xi Wang
2011-11-27 2:52 ` Greg KH
0 siblings, 1 reply; 14+ messages in thread
From: Xi Wang @ 2011-11-25 21:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Mori Hess, security, Lars-Peter Clausen, Ian Abbott,
Lucas De Marchi, Greg Kroah-Hartman, linux-kernel, Ian Abbott,
Franky Lin, Greg Dietsche, Mark Pearson
There is a potential integer overflow in do_insnlist_ioctl() if
userspace passes in a large insnlist.n_insns. The call to kmalloc()
would allocate a small buffer, leading to a memory corruption.
The bug was reported by Dan Carpenter <dan.carpenter@oracle.com>
and Haogang Chen <haogangchen@gmail.com>. The patch was suggested by
Ian Abbott <abbotti@mev.co.uk> and Lars-Peter Clausen <lars@metafoo.de>.
Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
drivers/staging/comedi/comedi_fops.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/comedi/comedi_fops.c
b/drivers/staging/comedi/comedi_fops.c
index 21d8c1c..7f7d79e 100644
--- a/drivers/staging/comedi/comedi_fops.c
+++ b/drivers/staging/comedi/comedi_fops.c
@@ -671,7 +671,7 @@ static int do_insnlist_ioctl(struct comedi_device *dev,
}
insns =
- kmalloc(sizeof(struct comedi_insn) * insnlist.n_insns, GFP_KERNEL);
+ kcalloc(insnlist.n_insns, sizeof(struct comedi_insn), GFP_KERNEL);
if (!insns) {
DPRINTK("kmalloc failed\n");
ret = -ENOMEM;
--
1.7.5.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] comedi: integer overflow in do_insnlist_ioctl()
2011-11-25 21:46 ` [PATCH v3] " Xi Wang
@ 2011-11-27 2:52 ` Greg KH
2011-11-27 11:25 ` Dan Carpenter
0 siblings, 1 reply; 14+ messages in thread
From: Greg KH @ 2011-11-27 2:52 UTC (permalink / raw)
To: Xi Wang
Cc: Dan Carpenter, devel, Mori Hess, security, Lars-Peter Clausen,
Ian Abbott, Lucas De Marchi, Greg Kroah-Hartman, linux-kernel,
Ian Abbott, Franky Lin, Greg Dietsche, Mark Pearson
On Fri, Nov 25, 2011 at 04:46:51PM -0500, Xi Wang wrote:
> There is a potential integer overflow in do_insnlist_ioctl() if
> userspace passes in a large insnlist.n_insns. The call to kmalloc()
> would allocate a small buffer, leading to a memory corruption.
>
> The bug was reported by Dan Carpenter <dan.carpenter@oracle.com>
> and Haogang Chen <haogangchen@gmail.com>. The patch was suggested by
> Ian Abbott <abbotti@mev.co.uk> and Lars-Peter Clausen <lars@metafoo.de>.
>
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
Hm, I already applied Dan's previous patch, what should I do with this
one now? Revert Dan's and apply this one, or apply both of them, or
something else?
confused,
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] comedi: integer overflow in do_insnlist_ioctl()
2011-11-27 2:52 ` Greg KH
@ 2011-11-27 11:25 ` Dan Carpenter
2011-11-27 21:24 ` Greg KH
0 siblings, 1 reply; 14+ messages in thread
From: Dan Carpenter @ 2011-11-27 11:25 UTC (permalink / raw)
To: Greg KH
Cc: Xi Wang, devel, Mori Hess, security, Lars-Peter Clausen,
Ian Abbott, Lucas De Marchi, Greg Kroah-Hartman, linux-kernel,
Ian Abbott, Franky Lin, Greg Dietsche, Mark Pearson
[-- Attachment #1: Type: text/plain, Size: 946 bytes --]
On Sat, Nov 26, 2011 at 06:52:52PM -0800, Greg KH wrote:
> On Fri, Nov 25, 2011 at 04:46:51PM -0500, Xi Wang wrote:
> > There is a potential integer overflow in do_insnlist_ioctl() if
> > userspace passes in a large insnlist.n_insns. The call to kmalloc()
> > would allocate a small buffer, leading to a memory corruption.
> >
> > The bug was reported by Dan Carpenter <dan.carpenter@oracle.com>
> > and Haogang Chen <haogangchen@gmail.com>. The patch was suggested by
> > Ian Abbott <abbotti@mev.co.uk> and Lars-Peter Clausen <lars@metafoo.de>.
> >
> > Signed-off-by: Xi Wang <xi.wang@gmail.com>
>
> Hm, I already applied Dan's previous patch, what should I do with this
> one now? Revert Dan's and apply this one, or apply both of them, or
> something else?
Sorry for that, I should have replied to my patch when I learned that
it had a problem.
Please, revert mine and apply Xi Wang's.
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] comedi: integer overflow in do_insnlist_ioctl()
2011-11-27 11:25 ` Dan Carpenter
@ 2011-11-27 21:24 ` Greg KH
0 siblings, 0 replies; 14+ messages in thread
From: Greg KH @ 2011-11-27 21:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: Xi Wang, devel, Mori Hess, security, Lars-Peter Clausen,
Ian Abbott, Lucas De Marchi, Greg Kroah-Hartman, linux-kernel,
Ian Abbott, Franky Lin, Greg Dietsche, Mark Pearson
On Sun, Nov 27, 2011 at 02:25:39PM +0300, Dan Carpenter wrote:
> On Sat, Nov 26, 2011 at 06:52:52PM -0800, Greg KH wrote:
> > On Fri, Nov 25, 2011 at 04:46:51PM -0500, Xi Wang wrote:
> > > There is a potential integer overflow in do_insnlist_ioctl() if
> > > userspace passes in a large insnlist.n_insns. The call to kmalloc()
> > > would allocate a small buffer, leading to a memory corruption.
> > >
> > > The bug was reported by Dan Carpenter <dan.carpenter@oracle.com>
> > > and Haogang Chen <haogangchen@gmail.com>. The patch was suggested by
> > > Ian Abbott <abbotti@mev.co.uk> and Lars-Peter Clausen <lars@metafoo.de>.
> > >
> > > Signed-off-by: Xi Wang <xi.wang@gmail.com>
> >
> > Hm, I already applied Dan's previous patch, what should I do with this
> > one now? Revert Dan's and apply this one, or apply both of them, or
> > something else?
>
> Sorry for that, I should have replied to my patch when I learned that
> it had a problem.
>
> Please, revert mine and apply Xi Wang's.
Ok, now done.
greg k-h
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-11-27 21:25 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-23 0:49 [PATCH] comedi: integer overflow in do_insnlist_ioctl() Xi Wang
2011-11-23 6:13 ` Dan Carpenter
2011-11-23 13:59 ` Xi Wang
2011-11-23 14:50 ` Dan Carpenter
2011-11-23 16:06 ` Ian Abbott
2011-11-23 16:53 ` [PATCH v2] " Xi Wang
2011-11-23 21:41 ` [PATCH] " Lars-Peter Clausen
2011-11-23 21:51 ` Dan Carpenter
2011-11-24 19:07 ` Xi Wang
2011-11-25 7:25 ` Dan Carpenter
2011-11-25 21:46 ` [PATCH v3] " Xi Wang
2011-11-27 2:52 ` Greg KH
2011-11-27 11:25 ` Dan Carpenter
2011-11-27 21:24 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.