* Re: [PATCH] firewire: prevent integer overflow on 32bit systems
2021-03-02 11:18 [PATCH] firewire: prevent integer overflow on 32bit systems Dan Carpenter
@ 2021-03-02 21:19 ` Stefan Richter
2021-03-03 5:22 ` Dan Carpenter
2021-03-03 17:31 ` kernel test robot
1 sibling, 1 reply; 4+ messages in thread
From: Stefan Richter @ 2021-03-02 21:19 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux1394-devel, linux-kernel, kernel-janitors
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 <dan.carpenter@oracle.com>
> ---
> 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/
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] firewire: prevent integer overflow on 32bit systems
2021-03-02 11:18 [PATCH] firewire: prevent integer overflow on 32bit systems Dan Carpenter
2021-03-02 21:19 ` Stefan Richter
@ 2021-03-03 17:31 ` kernel test robot
1 sibling, 0 replies; 4+ messages in thread
From: kernel test robot @ 2021-03-03 17:31 UTC (permalink / raw)
To: Dan Carpenter, Stefan Richter
Cc: kbuild-all, clang-built-linux, linux1394-devel, linux-kernel,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36079 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread