| Message ID | 20240216134541.31577-1-a.burakov@rosalinux.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: fix memory leak in __cld_pipe_inprogress_downcall() | expand |
Hello Aleksandr, On 24/02/16 04:45PM, Aleksandr Burakov wrote: > Dynamic memory, referenced by 'princhash.data' and 'name.data', > is allocated by calling function 'memdup_user' and lost > at __cld_pipe_inprogress_downcall() function return It is not actually lost. If nfs4_client_to_reclaim() fails and thus returns NULL - this error case is already properly handled. If nfs4_client_to_reclaim() succeeds then reference to the memory in question is passed to crp->cr_name.data and crp->cr_princhash.data correspondingly, and crp->cr_strhash entry is added to the list associated with nfsd_net. In this case the memory is supposed to be freed by nfs4_remove_reclaim_record(). See comment for nfs4_client_to_reclaim(). So I think the patch just introduces a double-free. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 11a60d159259 ("nfsd: add a "GetVersion" upcall for nfsdcld") > Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru> > --- > fs/nfsd/nfs4recover.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > index 2c060e0b1604..02663484782d 100644 > --- a/fs/nfsd/nfs4recover.c > +++ b/fs/nfsd/nfs4recover.c > @@ -850,6 +850,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > kfree(princhash.data); > return -EFAULT; > } > + kfree(name.data); > + kfree(princhash.data); > return nn->client_tracking_ops->msglen; > } > return -EFAULT; > -- > 2.25.1 -- Fedor
On Mon, 19 Feb 2024, Fedor Pchelkin wrote: > Hello Aleksandr, > > On 24/02/16 04:45PM, Aleksandr Burakov wrote: > > Dynamic memory, referenced by 'princhash.data' and 'name.data', > > is allocated by calling function 'memdup_user' and lost > > at __cld_pipe_inprogress_downcall() function return > > It is not actually lost. If nfs4_client_to_reclaim() fails and thus > returns NULL - this error case is already properly handled. > > If nfs4_client_to_reclaim() succeeds then reference to the memory in > question is passed to crp->cr_name.data and crp->cr_princhash.data > correspondingly, and crp->cr_strhash entry is added to the list associated > with nfsd_net. In this case the memory is supposed to be freed by > nfs4_remove_reclaim_record(). See comment for nfs4_client_to_reclaim(). > > So I think the patch just introduces a double-free. Agreed - this patch is incorrect. Thanks, NeilBrown > > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > Fixes: 11a60d159259 ("nfsd: add a "GetVersion" upcall for nfsdcld") > > Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru> > > --- > > fs/nfsd/nfs4recover.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c > > index 2c060e0b1604..02663484782d 100644 > > --- a/fs/nfsd/nfs4recover.c > > +++ b/fs/nfsd/nfs4recover.c > > @@ -850,6 +850,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, > > kfree(princhash.data); > > return -EFAULT; > > } > > + kfree(name.data); > > + kfree(princhash.data); > > return nn->client_tracking_ops->msglen; > > } > > return -EFAULT; > > -- > > 2.25.1 > > -- > Fedor > >
diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c index 2c060e0b1604..02663484782d 100644 --- a/fs/nfsd/nfs4recover.c +++ b/fs/nfsd/nfs4recover.c @@ -850,6 +850,8 @@ __cld_pipe_inprogress_downcall(const struct cld_msg_v2 __user *cmsg, kfree(princhash.data); return -EFAULT; } + kfree(name.data); + kfree(princhash.data); return nn->client_tracking_ops->msglen; } return -EFAULT;
Dynamic memory, referenced by 'princhash.data' and 'name.data', is allocated by calling function 'memdup_user' and lost at __cld_pipe_inprogress_downcall() function return Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 11a60d159259 ("nfsd: add a "GetVersion" upcall for nfsdcld") Signed-off-by: Aleksandr Burakov <a.burakov@rosalinux.ru> --- fs/nfsd/nfs4recover.c | 2 ++ 1 file changed, 2 insertions(+)