| Message ID | 122a5729fdcd76e23641c7d1853de2a632f6a742.1661509473.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfsd: Fix a memory leak in an error handling path | expand |
On Fri, 2022-08-26 at 12:24 +0200, Christophe JAILLET wrote: > If this memdup_user() call fails, the memory allocated in a previous call > a few lines above should be freed. Otherwise it leaks. > > Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Speculative, untested. > --- > fs/nfsd/nfs4recover.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > index b29d27eaa8a6..248ff9f4141c 100644 > --- a/fs/nfsd/nfs4recover.c > +++ b/fs/nfsd/nfs4recover.c > @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > princhash.data = memdup_user( > &ci->cc_princhash.cp_data, > princhashlen); > - if (IS_ERR_OR_NULL(princhash.data)) > + if (IS_ERR_OR_NULL(princhash.data)) { > + kfree(name.data); > return -EFAULT; > + } > princhash.len = princhashlen; > } else > princhash.len = 0; Reviewed-by: Jeff Layton <jlayton@kernel.org>
On Fri, Aug 26, 2022 at 12:24:54PM +0200, Christophe JAILLET wrote: > If this memdup_user() call fails, the memory allocated in a previous call > a few lines above should be freed. Otherwise it leaks. > > Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Speculative, untested. > --- > fs/nfsd/nfs4recover.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > index b29d27eaa8a6..248ff9f4141c 100644 > --- a/fs/nfsd/nfs4recover.c > +++ b/fs/nfsd/nfs4recover.c > @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > princhash.data = memdup_user( > &ci->cc_princhash.cp_data, > princhashlen); > - if (IS_ERR_OR_NULL(princhash.data)) > + if (IS_ERR_OR_NULL(princhash.data)) { > + kfree(name.data); > return -EFAULT; This comment is not directed at you and is not related to your patch. But memdup_user() never returns NULL, only error pointers. I wrote a fifteen page blog entry about NULL vs error pointers the other week. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ This should propagate the error code from memdup_user() instead of -EFAULT. regards, dan carpenter
> On Aug 26, 2022, at 7:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Fri, Aug 26, 2022 at 12:24:54PM +0200, Christophe JAILLET wrote: >> If this memdup_user() call fails, the memory allocated in a previous call >> a few lines above should be freed. Otherwise it leaks. >> >> Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> Speculative, untested. >> --- >> fs/nfsd/nfs4recover.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c >> index b29d27eaa8a6..248ff9f4141c 100644 >> --- a/fs/nfsd/nfs4recover.c >> +++ b/fs/nfsd/nfs4recover.c >> @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, >> princhash.data = memdup_user( >> &ci->cc_princhash.cp_data, >> princhashlen); >> - if (IS_ERR_OR_NULL(princhash.data)) >> + if (IS_ERR_OR_NULL(princhash.data)) { >> + kfree(name.data); >> return -EFAULT; > > This comment is not directed at you and is not related to your patch. > But memdup_user() never returns NULL, only error pointers. I wrote a > fifteen page blog entry about NULL vs error pointers the other week. > https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ > This should propagate the error code from memdup_user() instead of > -EFAULT. I take it then that Christophe should redrive this with your suggested corrections? I haven't applied this yet because I was waiting for follow-up. -- Chuck Lever
Le 30/08/2022 à 23:11, Chuck Lever III a écrit : > > >> On Aug 26, 2022, at 7:08 AM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote: >> >> On Fri, Aug 26, 2022 at 12:24:54PM +0200, Christophe JAILLET wrote: >>> If this memdup_user() call fails, the memory allocated in a previous call >>> a few lines above should be freed. Otherwise it leaks. >>> >>> Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") >>> Signed-off-by: Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> >>> --- >>> Speculative, untested. >>> --- >>> fs/nfsd/nfs4recover.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c >>> index b29d27eaa8a6..248ff9f4141c 100644 >>> --- a/fs/nfsd/nfs4recover.c >>> +++ b/fs/nfsd/nfs4recover.c >>> @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, >>> princhash.data = memdup_user( >>> &ci->cc_princhash.cp_data, >>> princhashlen); >>> - if (IS_ERR_OR_NULL(princhash.data)) >>> + if (IS_ERR_OR_NULL(princhash.data)) { >>> + kfree(name.data); >>> return -EFAULT; >> >> This comment is not directed at you and is not related to your patch. >> But memdup_user() never returns NULL, only error pointers. I wrote a >> fifteen page blog entry about NULL vs error pointers the other week. >> https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ >> This should propagate the error code from memdup_user() instead of >> -EFAULT. > > I take it then that Christophe should redrive this with your suggested > corrections? I haven't applied this yet because I was waiting for > follow-up. > Ok, I'll send a small serie of 3 patches... CJ > > -- > Chuck Lever > > > >
On Tue, Aug 30, 2022 at 09:11:54PM +0000, Chuck Lever III wrote: > > > > On Aug 26, 2022, at 7:08 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Fri, Aug 26, 2022 at 12:24:54PM +0200, Christophe JAILLET wrote: > >> If this memdup_user() call fails, the memory allocated in a previous call > >> a few lines above should be freed. Otherwise it leaks. > >> > >> Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > >> --- > >> Speculative, untested. > >> --- > >> fs/nfsd/nfs4recover.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > >> index b29d27eaa8a6..248ff9f4141c 100644 > >> --- a/fs/nfsd/nfs4recover.c > >> +++ b/fs/nfsd/nfs4recover.c > >> @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > >> princhash.data = memdup_user( > >> &ci->cc_princhash.cp_data, > >> princhashlen); > >> - if (IS_ERR_OR_NULL(princhash.data)) > >> + if (IS_ERR_OR_NULL(princhash.data)) { > >> + kfree(name.data); > >> return -EFAULT; > > > > This comment is not directed at you and is not related to your patch. > > But memdup_user() never returns NULL, only error pointers. I wrote a > > fifteen page blog entry about NULL vs error pointers the other week. > > https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/ > > This should propagate the error code from memdup_user() instead of > > -EFAULT. > > I take it then that Christophe should redrive this with your suggested > corrections? I haven't applied this yet because I was waiting for > follow-up. No, that's a different thing. If anyone wants to clean that up then it should be part of a different patch. regards, dan carpenter
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index b29d27eaa8a6..248ff9f4141c 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -815,8 +815,10 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, princhash.data = memdup_user( &ci->cc_princhash.cp_data, princhashlen); - if (IS_ERR_OR_NULL(princhash.data)) + if (IS_ERR_OR_NULL(princhash.data)) { + kfree(name.data); return -EFAULT; + } princhash.len = princhashlen; } else princhash.len = 0;
If this memdup_user() call fails, the memory allocated in a previous call a few lines above should be freed. Otherwise it leaks. Fixes: 6ee95d1c8991 ("nfsd: add support for upcall version 2") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Speculative, untested. --- fs/nfsd/nfs4recover.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)