[PATCH] firewire: prevent integer overflow on 32bit systems

[PATCH] firewire: prevent integer overflow on 32bit systems

[Dan Carpenter]

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;
-- 
2.30.1

Re: [PATCH] firewire: prevent integer overflow on 32bit systems

[Stefan Richter]

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/

Re: [PATCH] firewire: prevent integer overflow on 32bit systems

[Dan Carpenter]

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 <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().

Oh, yeah.  You're right.  I should have looked more carefully.  Sorry.

regards,
dan carpenter

Re: [PATCH] firewire: prevent integer overflow on 32bit systems

[kernel test robot]

[-- 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 --]