| Message ID | 20241014113416.2280986-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: client: fix possible double free in smb2_set_ea() | expand |
On Mon, Oct 14, 2024 at 07:34:17PM +0800, Su Hui wrote: > Clang static checker(scan-build) warning： > fs/smb/client/smb2ops.c:1304:2: Attempt to free released memory. > 1304 | kfree(ea); > | ^~~~~~~~~ > > There is a double free in such case: > 'ea is initialized to NULL' -> 'first successful memory allocation for > ea' -> 'something failed, goto sea_exit' -> 'first memory release for ea' > -> 'goto replay_again' -> 'second goto sea_exit before allocate memory > for ea' -> 'second memory release for ea resulted in double free'. > > Assign NULL value to 'ea' after 'kfree(ea)', it can fix this double free > problem. > > Fixes: 4f1fffa23769 ("cifs: commands that are retried should have replay flag set") > Signed-off-by: Su Hui <suhui@nfschina.com> > --- > fs/smb/client/smb2ops.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c > index 6b385fce3f2a..5b42b352b703 100644 > --- a/fs/smb/client/smb2ops.c > +++ b/fs/smb/client/smb2ops.c > @@ -1302,6 +1302,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon, > > sea_exit: > kfree(ea); > + ea = NULL; That's very clever. But I think that it would be better to do the "ea = NULL" near to the replay_again label. There are some lines where we re-initialize resp_buftype[], utf16_path and vars etc. ea = NULL; resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER; regards, dan carpenter
On 2024/10/14 20:53, Dan Carpenter wrote: > On Mon, Oct 14, 2024 at 07:34:17PM +0800, Su Hui wrote: >> Clang static checker(scan-build) warning： >> fs/smb/client/smb2ops.c:1304:2: Attempt to free released memory. >> 1304 | kfree(ea); >> | ^~~~~~~~~ >> >> There is a double free in such case: >> 'ea is initialized to NULL' -> 'first successful memory allocation for >> ea' -> 'something failed, goto sea_exit' -> 'first memory release for ea' >> -> 'goto replay_again' -> 'second goto sea_exit before allocate memory >> for ea' -> 'second memory release for ea resulted in double free'. >> >> Assign NULL value to 'ea' after 'kfree(ea)', it can fix this double free >> problem. >> >> Fixes: 4f1fffa23769 ("cifs: commands that are retried should have replay flag set") >> Signed-off-by: Su Hui <suhui@nfschina.com> >> --- >> fs/smb/client/smb2ops.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c >> index 6b385fce3f2a..5b42b352b703 100644 >> --- a/fs/smb/client/smb2ops.c >> +++ b/fs/smb/client/smb2ops.c >> @@ -1302,6 +1302,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon, >> >> sea_exit: >> kfree(ea); >> + ea = NULL; > That's very clever. But I think that it would be better to do the "ea = NULL" > near to the replay_again label. There are some lines where we re-initialize > resp_buftype[], utf16_path and vars etc. > > ea = NULL; > resp_buftype[0] = resp_buftype[1] = resp_buftype[2] = CIFS_NO_BUFFER; Yes, it's better! Thanks for your suggestions. I will send v2 patch soon. Su Hui
diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c index 6b385fce3f2a..5b42b352b703 100644 --- a/fs/smb/client/smb2ops.c +++ b/fs/smb/client/smb2ops.c @@ -1302,6 +1302,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon, sea_exit: kfree(ea); + ea = NULL; SMB2_open_free(&rqst[0]); SMB2_set_info_free(&rqst[1]); SMB2_close_free(&rqst[2]);
Clang static checker(scan-build) warning： fs/smb/client/smb2ops.c:1304:2: Attempt to free released memory. 1304 | kfree(ea); | ^~~~~~~~~ There is a double free in such case: 'ea is initialized to NULL' -> 'first successful memory allocation for ea' -> 'something failed, goto sea_exit' -> 'first memory release for ea' -> 'goto replay_again' -> 'second goto sea_exit before allocate memory for ea' -> 'second memory release for ea resulted in double free'. Assign NULL value to 'ea' after 'kfree(ea)', it can fix this double free problem. Fixes: 4f1fffa23769 ("cifs: commands that are retried should have replay flag set") Signed-off-by: Su Hui <suhui@nfschina.com> --- fs/smb/client/smb2ops.c | 1 + 1 file changed, 1 insertion(+)