[PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array

[PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array

[Zhipeng Lu]

The creds and oa->data need to be freed in the error-handling paths after
there allocation. So this patch add these deallocations in the
corresponding paths.

Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
---
 net/sunrpc/auth_gss/gss_rpc_xdr.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index d79f12c2550a..de533b20231b 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -250,8 +250,8 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
 
 	creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
 	if (!creds) {
-		kfree(oa->data);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto free_oa;
 	}
 
 	oa->data[0].option.data = CREDS_VALUE;
@@ -265,29 +265,41 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
 
 		/* option buffer */
 		p = xdr_inline_decode(xdr, 4);
-		if (unlikely(p == NULL))
-			return -ENOSPC;
+		if (unlikely(p == NULL)) {
+			err = -ENOSPC
+			goto free_creds;
+		}
 
 		length = be32_to_cpup(p);
 		p = xdr_inline_decode(xdr, length);
-		if (unlikely(p == NULL))
-			return -ENOSPC;
+		if (unlikely(p == NULL)) {
+			err = -ENOSPC
+			goto free_creds;
+		}
 
 		if (length == sizeof(CREDS_VALUE) &&
 		    memcmp(p, CREDS_VALUE, sizeof(CREDS_VALUE)) == 0) {
 			/* We have creds here. parse them */
 			err = gssx_dec_linux_creds(xdr, creds);
 			if (err)
-				return err;
+				goto free_creds;
 			oa->data[0].value.len = 1; /* presence */
 		} else {
 			/* consume uninteresting buffer */
 			err = gssx_dec_buffer(xdr, &dummy);
 			if (err)
-				return err;
+				goto free_creds;
 		}
 	}
 	return 0;
+
+free_creds:
+	kfree(creds);
+free_oa:
+	kfree(oa->data);
+	oa->data = NULL;
+err:
+	return err;
 }
 
 static int gssx_dec_status(struct xdr_stream *xdr,
-- 
2.34.1

Re: [PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array

[Simon Horman]

On Sun, Dec 24, 2023 at 04:24:22PM +0800, Zhipeng Lu wrote:
> The creds and oa->data need to be freed in the error-handling paths after
> there allocation. So this patch add these deallocations in the
> corresponding paths.
> 
> Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>

...

> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c

...

> @@ -265,29 +265,41 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>  
>  		/* option buffer */
>  		p = xdr_inline_decode(xdr, 4);
> -		if (unlikely(p == NULL))
> -			return -ENOSPC;
> +		if (unlikely(p == NULL)) {
> +			err = -ENOSPC

Hi Zhipeng Lu,

unfortunately the line above causes a build failure.

...

Re: Re: [PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array

[alexious]

> 
> On Sun, Dec 24, 2023 at 04:24:22PM +0800, Zhipeng Lu wrote:
> > The creds and oa->data need to be freed in the error-handling paths after
> > there allocation. So this patch add these deallocations in the
> > corresponding paths.
> > 
> > Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
> > Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
> 
> ...
> 
> > diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> 
> ...
> 
> > @@ -265,29 +265,41 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
> >  
> >  		/* option buffer */
> >  		p = xdr_inline_decode(xdr, 4);
> > -		if (unlikely(p == NULL))
> > -			return -ENOSPC;
> > +		if (unlikely(p == NULL)) {
> > +			err = -ENOSPC
> 
> Hi Zhipeng Lu,
> 
> unfortunately the line above causes a build failure.
> 
> ...

Sorry for my mistake, I'll send a version 2 of this patch soon.

Re: [PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array

[kernel test robot]

Hi Zhipeng,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.7-rc7 next-20231222]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zhipeng-Lu/SUNRPC-fix-some-memleaks-in-gssx_dec_option_array/20231225-152918
base:   linus/master
patch link:    https://lore.kernel.org/r/20231224082424.3539726-1-alexious%40zju.edu.cn
patch subject: [PATCH] SUNRPC: fix some memleaks in gssx_dec_option_array
config: nios2-randconfig-r081-20231225 (https://download.01.org/0day-ci/archive/20231226/202312260138.JJkoofSt-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231226/202312260138.JJkoofSt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312260138.JJkoofSt-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   net/sunrpc/auth_gss/gss_rpc_xdr.c: In function 'gssx_dec_option_array':
>> net/sunrpc/auth_gss/gss_rpc_xdr.c:270:25: error: expected ';' before 'goto'
     270 |                         goto free_creds;
         |                         ^~~~
   net/sunrpc/auth_gss/gss_rpc_xdr.c:277:25: error: expected ';' before 'goto'
     277 |                         goto free_creds;
         |                         ^~~~
>> net/sunrpc/auth_gss/gss_rpc_xdr.c:301:1: warning: label 'err' defined but not used [-Wunused-label]
     301 | err:
         | ^~~


vim +270 net/sunrpc/auth_gss/gss_rpc_xdr.c

   228	
   229	static int gssx_dec_option_array(struct xdr_stream *xdr,
   230					 struct gssx_option_array *oa)
   231	{
   232		struct svc_cred *creds;
   233		u32 count, i;
   234		__be32 *p;
   235		int err;
   236	
   237		p = xdr_inline_decode(xdr, 4);
   238		if (unlikely(p == NULL))
   239			return -ENOSPC;
   240		count = be32_to_cpup(p++);
   241		if (!count)
   242			return 0;
   243	
   244		/* we recognize only 1 currently: CREDS_VALUE */
   245		oa->count = 1;
   246	
   247		oa->data = kmalloc(sizeof(struct gssx_option), GFP_KERNEL);
   248		if (!oa->data)
   249			return -ENOMEM;
   250	
   251		creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
   252		if (!creds) {
   253			err = -ENOMEM;
   254			goto free_oa;
   255		}
   256	
   257		oa->data[0].option.data = CREDS_VALUE;
   258		oa->data[0].option.len = sizeof(CREDS_VALUE);
   259		oa->data[0].value.data = (void *)creds;
   260		oa->data[0].value.len = 0;
   261	
   262		for (i = 0; i < count; i++) {
   263			gssx_buffer dummy = { 0, NULL };
   264			u32 length;
   265	
   266			/* option buffer */
   267			p = xdr_inline_decode(xdr, 4);
   268			if (unlikely(p == NULL)) {
   269				err = -ENOSPC
 > 270				goto free_creds;
   271			}
   272	
   273			length = be32_to_cpup(p);
   274			p = xdr_inline_decode(xdr, length);
   275			if (unlikely(p == NULL)) {
   276				err = -ENOSPC
   277				goto free_creds;
   278			}
   279	
   280			if (length == sizeof(CREDS_VALUE) &&
   281			    memcmp(p, CREDS_VALUE, sizeof(CREDS_VALUE)) == 0) {
   282				/* We have creds here. parse them */
   283				err = gssx_dec_linux_creds(xdr, creds);
   284				if (err)
   285					goto free_creds;
   286				oa->data[0].value.len = 1; /* presence */
   287			} else {
   288				/* consume uninteresting buffer */
   289				err = gssx_dec_buffer(xdr, &dummy);
   290				if (err)
   291					goto free_creds;
   292			}
   293		}
   294		return 0;
   295	
   296	free_creds:
   297		kfree(creds);
   298	free_oa:
   299		kfree(oa->data);
   300		oa->data = NULL;
 > 301	err:
   302		return err;
   303	}
   304	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki