In TCODE_STREAM_DATA mode, on 32bit systems, the "sizeof(*e) +
request->length" operation can overflow leading to memory corruption.
Fixes: 18e9b10fcdc0 ("firewire: cdev: add closure to async stream ioctl")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/firewire/core-cdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index fb6c651214f3..314de0384035 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -587,6 +587,9 @@ static int init_request(struct client *client,
request->length < 4)
return -EINVAL;
+ if (request->length > ULONG_MAX - sizeof(*e))
+ return -EINVAL;
+
e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL);
if (e == NULL)
return -ENOMEM;
--
2.30.1
On Mar 02 Dan Carpenter wrote:
> In TCODE_STREAM_DATA mode, on 32bit systems, the "sizeof(*e) +
> request->length" operation can overflow leading to memory corruption.
>
> Fixes: 18e9b10fcdc0 ("firewire: cdev: add closure to async stream ioctl")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/firewire/core-cdev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> index fb6c651214f3..314de0384035 100644
> --- a/drivers/firewire/core-cdev.c
> +++ b/drivers/firewire/core-cdev.c
> @@ -587,6 +587,9 @@ static int init_request(struct client *client,
> request->length < 4)
> return -EINVAL;
>
> + if (request->length > ULONG_MAX - sizeof(*e))
> + return -EINVAL;
> +
> e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL);
> if (e == NULL)
> return -ENOMEM;
There is already a length check for asynchronous stream requests.
It happens in ioctl_send_stream_packet().
--
Stefan Richter
-======--=-= --== ---=-
http://arcgraph.de/sr/
Hi Dan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.12-rc1 next-20210303]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Dan-Carpenter/firewire-prevent-integer-overflow-on-32bit-systems/20210303-201128
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2ab38c17aac10bf55ab3efde4c4db3893d8691d2
config: x86_64-randconfig-a006-20210303 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project a7cad6680b4087eff8994f1f99ac40c661a6621f)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/4571db3d737ee0538b14fcbc040f1b1284977386
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Dan-Carpenter/firewire-prevent-integer-overflow-on-32bit-systems/20210303-201128
git checkout 4571db3d737ee0538b14fcbc040f1b1284977386
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/firewire/core-cdev.c:590:22: warning: result of comparison of constant 18446744073709551279 with expression of type '__u32' (aka 'unsigned int') is always false [-Wtautological-constant-out-of-range-compare]
if (request->length > ULONG_MAX - sizeof(*e))
~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
vim +590 drivers/firewire/core-cdev.c
574
575 static int init_request(struct client *client,
576 struct fw_cdev_send_request *request,
577 int destination_id, int speed)
578 {
579 struct outbound_transaction_event *e;
580 int ret;
581
582 if (request->tcode != TCODE_STREAM_DATA &&
583 (request->length > 4096 || request->length > 512 << speed))
584 return -EIO;
585
586 if (request->tcode == TCODE_WRITE_QUADLET_REQUEST &&
587 request->length < 4)
588 return -EINVAL;
589
> 590 if (request->length > ULONG_MAX - sizeof(*e))
591 return -EINVAL;
592
593 e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL);
594 if (e == NULL)
595 return -ENOMEM;
596
597 e->client = client;
598 e->response.length = request->length;
599 e->response.closure = request->closure;
600
601 if (request->data &&
602 copy_from_user(e->response.data,
603 u64_to_uptr(request->data), request->length)) {
604 ret = -EFAULT;
605 goto failed;
606 }
607
608 e->r.resource.release = release_transaction;
609 ret = add_client_resource(client, &e->r.resource, GFP_KERNEL);
610 if (ret < 0)
611 goto failed;
612
613 fw_send_request(client->device->card, &e->r.transaction,
614 request->tcode, destination_id, request->generation,
615 speed, request->offset, e->response.data,
616 request->length, complete_transaction, e);
617 return 0;
618
619 failed:
620 kfree(e);
621
622 return ret;
623 }
624
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Tue, Mar 02, 2021 at 10:19:11PM +0100, Stefan Richter wrote:
> On Mar 02 Dan Carpenter wrote:
> > In TCODE_STREAM_DATA mode, on 32bit systems, the "sizeof(*e) +
> > request->length" operation can overflow leading to memory corruption.
> >
> > Fixes: 18e9b10fcdc0 ("firewire: cdev: add closure to async stream ioctl")
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > drivers/firewire/core-cdev.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
> > index fb6c651214f3..314de0384035 100644
> > --- a/drivers/firewire/core-cdev.c
> > +++ b/drivers/firewire/core-cdev.c
> > @@ -587,6 +587,9 @@ static int init_request(struct client *client,
> > request->length < 4)
> > return -EINVAL;
> >
> > + if (request->length > ULONG_MAX - sizeof(*e))
> > + return -EINVAL;
> > +
> > e = kmalloc(sizeof(*e) + request->length, GFP_KERNEL);
> > if (e == NULL)
> > return -ENOMEM;
>
> There is already a length check for asynchronous stream requests.
> It happens in ioctl_send_stream_packet().
Oh, yeah. You're right. I should have looked more carefully. Sorry.
regards,
dan carpenter