[PATCH 0/2] usb: gadget: Fix two memleaks in error handling

[PATCH 0/2] usb: gadget: Fix two memleaks in error handling

[Zhang Qilong]

This patch sets fix two memleaks before error returns.

Zhang Qilong (2):
  usb: gadget: f_midi: Fix memleak in f_midi_alloc
  usb: gadget: Fix memleak in gadgetfs_fill_super

 drivers/usb/gadget/function/f_midi.c | 3 +++
 drivers/usb/gadget/legacy/inode.c    | 3 +++
 2 files changed, 6 insertions(+)

-- 
2.25.4

[PATCH 1/2] usb: gadget: f_midi: Fix memleak in f_midi_alloc

[Zhang Qilong]

In the error path, if midi is not null, we should to
free the midi->id if necessary to prevent memleak.

Fixes: b85e9de9e818d ("usb: gadget: f_midi: convert to new function interface with backward compatibility")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/usb/gadget/function/f_midi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 85cb15734aa8..596fd7ce56fb 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1345,7 +1345,10 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
 
 setup_fail:
 	mutex_unlock(&opts->lock);
+	if (midi)
+		kfree(midi->id);
 	kfree(midi);
+
 	return ERR_PTR(status);
 }
 
-- 
2.25.4

[PATCH 2/2] usb: gadget: Fix memleak in gadgetfs_fill_super

[Zhang Qilong]

usb_get_gadget_udc_name will alloc memory for CHIP
in "Enomem" branch. we should free it before error
returns to prevent memleak.

Fixes: 175f712119c57 ("usb: gadget: provide interface for legacy gadgets to get UDC name")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/usb/gadget/legacy/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 1b430b36d0a6..3cefc27be098 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -2039,6 +2039,9 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
 	return 0;
 
 Enomem:
+	free(CHIP);
+	CHIP = NULL;
+
 	return -ENOMEM;
 }
 
-- 
2.25.4

Re: [PATCH 1/2] usb: gadget: f_midi: Fix memleak in f_midi_alloc

[Peter Chen]

On 20-11-16 20:17:09, Zhang Qilong wrote:
> In the error path, if midi is not null, we should to
> free the midi->id if necessary to prevent memleak.
> 
> Fixes: b85e9de9e818d ("usb: gadget: f_midi: convert to new function interface with backward compatibility")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> ---
>  drivers/usb/gadget/function/f_midi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 85cb15734aa8..596fd7ce56fb 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1345,7 +1345,10 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  
>  setup_fail:
>  	mutex_unlock(&opts->lock);
> +	if (midi)
> +		kfree(midi->id);
>  	kfree(midi);
> +
>  	return ERR_PTR(status);
>  }
>  

It is better to add another goto label for this memory free.

-- 

Thanks,
Peter Chen

Re: [PATCH 2/2] usb: gadget: Fix memleak in gadgetfs_fill_super

[Peter Chen]

On 20-11-16 20:17:10, Zhang Qilong wrote:
> usb_get_gadget_udc_name will alloc memory for CHIP
> in "Enomem" branch. we should free it before error
> returns to prevent memleak.
> 
> Fixes: 175f712119c57 ("usb: gadget: provide interface for legacy gadgets to get UDC name")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> ---
>  drivers/usb/gadget/legacy/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 1b430b36d0a6..3cefc27be098 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
> @@ -2039,6 +2039,9 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
>  	return 0;
>  
>  Enomem:
> +	free(CHIP);
> +	CHIP = NULL;
> +
>  	return -ENOMEM;
>  }

It seems the dev_new also allocate the memory, would you please change
it too, also, create a new goto entry for it.
-- 

Thanks,
Peter Chen

Re: [PATCH 2/2] usb: gadget: Fix memleak in gadgetfs_fill_super

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4000 bytes --]

Hi Zhang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on balbi-usb/testing/next]
[also build test ERROR on usb/usb-testing linus/master peter.chen-usb/ci-for-usb-next v5.10-rc4 next-20201116]
[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/Zhang-Qilong/usb-gadget-Fix-two-memleaks-in-error-handling/20201116-201612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: powerpc-randconfig-r015-20201116 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c044709b8fbea2a9a375e4173a6bd735f6866c0c)
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 powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/0dfab8598cb8b814fc17011d7a15ff463840e1dd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhang-Qilong/usb-gadget-Fix-two-memleaks-in-error-handling/20201116-201612
        git checkout 0dfab8598cb8b814fc17011d7a15ff463840e1dd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/gadget/legacy/inode.c:2042:2: error: implicitly declaring library function 'free' with type 'void (void *)' [-Werror,-Wimplicit-function-declaration]
           free(CHIP);
           ^
   drivers/usb/gadget/legacy/inode.c:2042:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
>> drivers/usb/gadget/legacy/inode.c:2042:7: error: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           free(CHIP);
                ^~~~
   2 errors generated.

vim +2042 drivers/usb/gadget/legacy/inode.c

  1990	
  1991	static int
  1992	gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
  1993	{
  1994		struct inode	*inode;
  1995		struct dev_data	*dev;
  1996	
  1997		if (the_device)
  1998			return -ESRCH;
  1999	
  2000		CHIP = usb_get_gadget_udc_name();
  2001		if (!CHIP)
  2002			return -ENODEV;
  2003	
  2004		/* superblock */
  2005		sb->s_blocksize = PAGE_SIZE;
  2006		sb->s_blocksize_bits = PAGE_SHIFT;
  2007		sb->s_magic = GADGETFS_MAGIC;
  2008		sb->s_op = &gadget_fs_operations;
  2009		sb->s_time_gran = 1;
  2010	
  2011		/* root inode */
  2012		inode = gadgetfs_make_inode (sb,
  2013				NULL, &simple_dir_operations,
  2014				S_IFDIR | S_IRUGO | S_IXUGO);
  2015		if (!inode)
  2016			goto Enomem;
  2017		inode->i_op = &simple_dir_inode_operations;
  2018		if (!(sb->s_root = d_make_root (inode)))
  2019			goto Enomem;
  2020	
  2021		/* the ep0 file is named after the controller we expect;
  2022		 * user mode code can use it for sanity checks, like we do.
  2023		 */
  2024		dev = dev_new ();
  2025		if (!dev)
  2026			goto Enomem;
  2027	
  2028		dev->sb = sb;
  2029		dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
  2030		if (!dev->dentry) {
  2031			put_dev(dev);
  2032			goto Enomem;
  2033		}
  2034	
  2035		/* other endpoint files are available after hardware setup,
  2036		 * from binding to a controller.
  2037		 */
  2038		the_device = dev;
  2039		return 0;
  2040	
  2041	Enomem:
> 2042		free(CHIP);
  2043		CHIP = NULL;
  2044	
  2045		return -ENOMEM;
  2046	}
  2047	

---
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: 33790 bytes --]

Re: [PATCH 2/2] usb: gadget: Fix memleak in gadgetfs_fill_super

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4106 bytes --]

Hi Zhang,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on balbi-usb/testing/next]
[also build test ERROR on usb/usb-testing linus/master peter.chen-usb/ci-for-usb-next v5.10-rc4 next-20201116]
[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/Zhang-Qilong/usb-gadget-Fix-two-memleaks-in-error-handling/20201116-201612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: powerpc-randconfig-r015-20201116 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c044709b8fbea2a9a375e4173a6bd735f6866c0c)
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 powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/0dfab8598cb8b814fc17011d7a15ff463840e1dd
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Zhang-Qilong/usb-gadget-Fix-two-memleaks-in-error-handling/20201116-201612
        git checkout 0dfab8598cb8b814fc17011d7a15ff463840e1dd
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/usb/gadget/legacy/inode.c:2042:2: error: implicitly declaring library function 'free' with type 'void (void *)' [-Werror,-Wimplicit-function-declaration]
           free(CHIP);
           ^
   drivers/usb/gadget/legacy/inode.c:2042:2: note: include the header <stdlib.h> or explicitly provide a declaration for 'free'
>> drivers/usb/gadget/legacy/inode.c:2042:7: error: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
           free(CHIP);
                ^~~~
   2 errors generated.

vim +2042 drivers/usb/gadget/legacy/inode.c

  1990	
  1991	static int
  1992	gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
  1993	{
  1994		struct inode	*inode;
  1995		struct dev_data	*dev;
  1996	
  1997		if (the_device)
  1998			return -ESRCH;
  1999	
  2000		CHIP = usb_get_gadget_udc_name();
  2001		if (!CHIP)
  2002			return -ENODEV;
  2003	
  2004		/* superblock */
  2005		sb->s_blocksize = PAGE_SIZE;
  2006		sb->s_blocksize_bits = PAGE_SHIFT;
  2007		sb->s_magic = GADGETFS_MAGIC;
  2008		sb->s_op = &gadget_fs_operations;
  2009		sb->s_time_gran = 1;
  2010	
  2011		/* root inode */
  2012		inode = gadgetfs_make_inode (sb,
  2013				NULL, &simple_dir_operations,
  2014				S_IFDIR | S_IRUGO | S_IXUGO);
  2015		if (!inode)
  2016			goto Enomem;
  2017		inode->i_op = &simple_dir_inode_operations;
  2018		if (!(sb->s_root = d_make_root (inode)))
  2019			goto Enomem;
  2020	
  2021		/* the ep0 file is named after the controller we expect;
  2022		 * user mode code can use it for sanity checks, like we do.
  2023		 */
  2024		dev = dev_new ();
  2025		if (!dev)
  2026			goto Enomem;
  2027	
  2028		dev->sb = sb;
  2029		dev->dentry = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
  2030		if (!dev->dentry) {
  2031			put_dev(dev);
  2032			goto Enomem;
  2033		}
  2034	
  2035		/* other endpoint files are available after hardware setup,
  2036		 * from binding to a controller.
  2037		 */
  2038		the_device = dev;
  2039		return 0;
  2040	
  2041	Enomem:
> 2042		free(CHIP);
  2043		CHIP = NULL;
  2044	
  2045		return -ENOMEM;
  2046	}
  2047	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33790 bytes --]

Re: [PATCH 1/2] usb: gadget: f_midi: Fix memleak in f_midi_alloc

[Sergei Shtylyov]

On 11/16/20 3:17 PM, Zhang Qilong wrote:

> In the error path, if midi is not null, we should to

   That "to" not needed here.

> free the midi->id if necessary to prevent memleak.
> 
> Fixes: b85e9de9e818d ("usb: gadget: f_midi: convert to new function interface with backward compatibility")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
[...]

MBR, Sergei

答复: [PATCH 2/2] usb: gadget: Fix memleak in gadgetfs_fill_super

[zhangqilong]

> 
> On 20-11-16 20:17:10, Zhang Qilong wrote:
> > usb_get_gadget_udc_name will alloc memory for CHIP in "Enomem" branch.
> > we should free it before error returns to prevent memleak.
> >
> > Fixes: 175f712119c57 ("usb: gadget: provide interface for legacy
> > gadgets to get UDC name")
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > ---
> >  drivers/usb/gadget/legacy/inode.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/usb/gadget/legacy/inode.c
> > b/drivers/usb/gadget/legacy/inode.c
> > index 1b430b36d0a6..3cefc27be098 100644
> > --- a/drivers/usb/gadget/legacy/inode.c
> > +++ b/drivers/usb/gadget/legacy/inode.c
> > @@ -2039,6 +2039,9 @@ gadgetfs_fill_super (struct super_block *sb, struct
> fs_context *fc)
> >  	return 0;
> >
> >  Enomem:
> > +	free(CHIP);
> > +	CHIP = NULL;
> > +
> >  	return -ENOMEM;
> >  }
> 
> It seems the dev_new also allocate the memory, would you please change it
> too, also, create a new goto entry for it.

If gadgetfs_create_file fails, the dev_new will be freed in put_dev.

Thanks,
Zhang

> --
> 
> Thanks,
> Peter Chen