* Re: [PATCH 0/5] cifs: Fix xid leak in cifs
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
@ 2022-10-17 13:53 ` Paulo Alcantara
2022-10-17 14:45 ` [PATCH 1/5] cifs: Fix xid leak in cifs_create() Zhang Xiaoxu
` (5 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Paulo Alcantara @ 2022-10-17 13:53 UTC (permalink / raw)
To: Zhang Xiaoxu, linux-cifs, zhangxiaoxu5, sfrench, smfrench,
lsahlber, sprasad, tom, aaptel
Zhang Xiaoxu <zhangxiaoxu5@huawei.com> writes:
> Zhang Xiaoxu (5):
> cifs: Fix xid leak in cifs_create()
> cifs: Fix xid leak in cifs_copy_file_range()
> cifs: Fix xid leak in cifs_flock()
> cifs: Fix xid leak in cifs_ses_add_channel()
> cifs: Fix xid leak in cifs_get_file_info_unix()
>
> fs/cifs/cifsfs.c | 7 +++++--
> fs/cifs/dir.c | 6 ++++--
> fs/cifs/file.c | 11 +++++++----
> fs/cifs/inode.c | 6 ++++--
> fs/cifs/sess.c | 1 +
> 5 files changed, 21 insertions(+), 10 deletions(-)
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/5] cifs: Fix xid leak in cifs_create()
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
2022-10-17 13:53 ` Paulo Alcantara
@ 2022-10-17 14:45 ` Zhang Xiaoxu
2022-10-17 14:45 ` [PATCH 2/5] cifs: Fix xid leak in cifs_copy_file_range() Zhang Xiaoxu
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-10-17 14:45 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, smfrench, pc, lsahlber,
sprasad, tom, aaptel
If the cifs already shutdown, we should free the xid before return,
otherwise, the xid will be leaked.
Fixes: 087f757b0129 ("cifs: add shutdown support")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
fs/cifs/dir.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index a5c73c2af3a2..8b1c37158556 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -543,8 +543,10 @@ int cifs_create(struct user_namespace *mnt_userns, struct inode *inode,
cifs_dbg(FYI, "cifs_create parent inode = 0x%p name is: %pd and dentry = 0x%p\n",
inode, direntry, direntry);
- if (unlikely(cifs_forced_shutdown(CIFS_SB(inode->i_sb))))
- return -EIO;
+ if (unlikely(cifs_forced_shutdown(CIFS_SB(inode->i_sb)))) {
+ rc = -EIO;
+ goto out_free_xid;
+ }
tlink = cifs_sb_tlink(CIFS_SB(inode->i_sb));
rc = PTR_ERR(tlink);
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/5] cifs: Fix xid leak in cifs_copy_file_range()
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
2022-10-17 13:53 ` Paulo Alcantara
2022-10-17 14:45 ` [PATCH 1/5] cifs: Fix xid leak in cifs_create() Zhang Xiaoxu
@ 2022-10-17 14:45 ` Zhang Xiaoxu
2022-10-17 14:45 ` [PATCH 3/5] cifs: Fix xid leak in cifs_flock() Zhang Xiaoxu
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-10-17 14:45 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, smfrench, pc, lsahlber,
sprasad, tom, aaptel
If the file is used by swap, before return -EOPNOTSUPP, should
free the xid, otherwise, the xid will be leaked.
Fixes: 4e8aea30f775 ("smb3: enable swap on SMB3 mounts")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
fs/cifs/cifsfs.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c6ac19223ddc..d0b9fec111aa 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1302,8 +1302,11 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
ssize_t rc;
struct cifsFileInfo *cfile = dst_file->private_data;
- if (cfile->swapfile)
- return -EOPNOTSUPP;
+ if (cfile->swapfile) {
+ rc = -EOPNOTSUPP;
+ free_xid(xid);
+ return rc;
+ }
rc = cifs_file_copychunk_range(xid, src_file, off, dst_file, destoff,
len, flags);
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/5] cifs: Fix xid leak in cifs_flock()
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
` (2 preceding siblings ...)
2022-10-17 14:45 ` [PATCH 2/5] cifs: Fix xid leak in cifs_copy_file_range() Zhang Xiaoxu
@ 2022-10-17 14:45 ` Zhang Xiaoxu
2022-10-17 14:45 ` [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel() Zhang Xiaoxu
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-10-17 14:45 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, smfrench, pc, lsahlber,
sprasad, tom, aaptel
If not flock, before return -ENOLCK, should free the xid,
otherwise, the xid will be leaked.
Fixes: d0677992d2af ("cifs: add support for flock")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
fs/cifs/file.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index f6ffee514c34..5b3b308e115c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -1885,11 +1885,13 @@ int cifs_flock(struct file *file, int cmd, struct file_lock *fl)
struct cifsFileInfo *cfile;
__u32 type;
- rc = -EACCES;
xid = get_xid();
- if (!(fl->fl_flags & FL_FLOCK))
- return -ENOLCK;
+ if (!(fl->fl_flags & FL_FLOCK)) {
+ rc = -ENOLCK;
+ free_xid(xid);
+ return rc;
+ }
cfile = (struct cifsFileInfo *)file->private_data;
tcon = tlink_tcon(cfile->tlink);
@@ -1908,8 +1910,9 @@ int cifs_flock(struct file *file, int cmd, struct file_lock *fl)
* if no lock or unlock then nothing to do since we do not
* know what it is
*/
+ rc = -EOPNOTSUPP;
free_xid(xid);
- return -EOPNOTSUPP;
+ return rc;
}
rc = cifs_setlk(file, fl, type, wait_flag, posix_lck, lock, unlock,
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel()
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
` (3 preceding siblings ...)
2022-10-17 14:45 ` [PATCH 3/5] cifs: Fix xid leak in cifs_flock() Zhang Xiaoxu
@ 2022-10-17 14:45 ` Zhang Xiaoxu
2022-10-17 17:28 ` Steve French
2022-10-17 14:45 ` [PATCH 5/5] cifs: Fix xid leak in cifs_get_file_info_unix() Zhang Xiaoxu
2022-10-17 17:28 ` [PATCH 0/5] cifs: Fix xid leak in cifs Steve French
6 siblings, 1 reply; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-10-17 14:45 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, smfrench, pc, lsahlber,
sprasad, tom, aaptel
Before return, should free the xid, otherwise, the
xid will be leaked.
Fixes: d70e9fa55884 ("cifs: try opening channels after mounting")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
fs/cifs/sess.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 0435d1dfa9e1..92e4278ec35d 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -496,6 +496,7 @@ cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,
cifs_put_tcp_session(chan->server, 0);
}
+ free_xid(xid);
return rc;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel()
2022-10-17 14:45 ` [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel() Zhang Xiaoxu
@ 2022-10-17 17:28 ` Steve French
0 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2022-10-17 17:28 UTC (permalink / raw)
To: Zhang Xiaoxu; +Cc: linux-cifs, sfrench, pc, lsahlber, sprasad, tom, aaptel
Your approach is probably better, but it would also possible to remove
the get_xid and pass 0 in to negotiate in this path
On Mon, Oct 17, 2022 at 8:42 AM Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote:
>
> Before return, should free the xid, otherwise, the
> xid will be leaked.
>
> Fixes: d70e9fa55884 ("cifs: try opening channels after mounting")
> Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
> ---
> fs/cifs/sess.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 0435d1dfa9e1..92e4278ec35d 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -496,6 +496,7 @@ cifs_ses_add_channel(struct cifs_sb_info *cifs_sb, struct cifs_ses *ses,
> cifs_put_tcp_session(chan->server, 0);
> }
>
> + free_xid(xid);
> return rc;
> }
>
> --
> 2.31.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] cifs: Fix xid leak in cifs_get_file_info_unix()
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
` (4 preceding siblings ...)
2022-10-17 14:45 ` [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel() Zhang Xiaoxu
@ 2022-10-17 14:45 ` Zhang Xiaoxu
2022-10-17 17:28 ` [PATCH 0/5] cifs: Fix xid leak in cifs Steve French
6 siblings, 0 replies; 9+ messages in thread
From: Zhang Xiaoxu @ 2022-10-17 14:45 UTC (permalink / raw)
To: linux-cifs, zhangxiaoxu5, sfrench, smfrench, pc, lsahlber,
sprasad, tom, aaptel
If stardup the symlink target failed, should free the xid,
otherwise the xid will be leaked.
Fixes: 76894f3e2f71 ("cifs: improve symlink handling for smb2+")
Signed-off-by: Zhang Xiaoxu <zhangxiaoxu5@huawei.com>
---
fs/cifs/inode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 7cf96e581d24..9bde08d44617 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -368,8 +368,10 @@ cifs_get_file_info_unix(struct file *filp)
if (cfile->symlink_target) {
fattr.cf_symlink_target = kstrdup(cfile->symlink_target, GFP_KERNEL);
- if (!fattr.cf_symlink_target)
- return -ENOMEM;
+ if (!fattr.cf_symlink_target) {
+ rc = -ENOMEM;
+ goto cifs_gfiunix_out;
+ }
}
rc = CIFSSMBUnixQFileInfo(xid, tcon, cfile->fid.netfid, &find_data);
--
2.31.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/5] cifs: Fix xid leak in cifs
2022-10-17 14:45 [PATCH 0/5] cifs: Fix xid leak in cifs Zhang Xiaoxu
` (5 preceding siblings ...)
2022-10-17 14:45 ` [PATCH 5/5] cifs: Fix xid leak in cifs_get_file_info_unix() Zhang Xiaoxu
@ 2022-10-17 17:28 ` Steve French
6 siblings, 0 replies; 9+ messages in thread
From: Steve French @ 2022-10-17 17:28 UTC (permalink / raw)
To: Zhang Xiaoxu; +Cc: linux-cifs, sfrench, pc, lsahlber, sprasad, tom, aaptel
Good catch - merged into cifs-2.6.git for-next
In one of the cases we also as an alternative could have skipped the
get_xid instead as an alternative (and passed zero as xid to negotiate
in that case) - but your approach may be slightly better
On Mon, Oct 17, 2022 at 8:42 AM Zhang Xiaoxu <zhangxiaoxu5@huawei.com> wrote:
>
> Found some xid leak with the following cocci script:
>
> /usr/bin/spatch -I include -timeout 60 -very_quiet \
> -sp_file missing-free_xid.cocci fs/cifs
>
> @r1@
> identifier xid;
> position p;
> @@
> ...
> xid = get_xid();
> <+... when != free_xid(xid)
> if (...) {
> ... when != free_xid(xid)
> when forall
> return@p ...;
> }
> ...+>
> free_xid(xid);
>
> @depends on r1@
> position r1.p;
> @@
> + free_xid(xid);
> return@p ...;
>
> @r2@
> identifier xid;
> position p;
> @@
> ...
> unsigned int xid = get_xid();
> <+... when != free_xid(xid)
> if (...) {
> ... when != free_xid(xid)
> when forall
> return@p ...;
> }
> ...+>
> free_xid(xid);
>
> @depends on r2@
> position r2.p;
> @@
> + free_xid(xid);
> return@p ...;
>
> @r3@
> identifier xid;
> position p;
> @@
> ...
> xid = get_xid();
> ... when != \(free_xid\|_free_xid\)(xid);
> return@p ...;
>
> @depends on r3@
> position r3.p;
> @@
> + free_xid(xid);
> return@p ...;
>
> @r4@
> identifier xid;
> position p;
> @@
> ...
> unsigned int xid = get_xid();
> ... when != \(free_xid\|_free_xid\)(xid);
> return@p ...;
>
> @depends on r4@
> position r4.p;
> @@
> + free_xid(xid);
> return@p ...;
>
> Zhang Xiaoxu (5):
> cifs: Fix xid leak in cifs_create()
> cifs: Fix xid leak in cifs_copy_file_range()
> cifs: Fix xid leak in cifs_flock()
> cifs: Fix xid leak in cifs_ses_add_channel()
> cifs: Fix xid leak in cifs_get_file_info_unix()
>
> fs/cifs/cifsfs.c | 7 +++++--
> fs/cifs/dir.c | 6 ++++--
> fs/cifs/file.c | 11 +++++++----
> fs/cifs/inode.c | 6 ++++--
> fs/cifs/sess.c | 1 +
> 5 files changed, 21 insertions(+), 10 deletions(-)
>
> --
> 2.31.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread