Re: [PATCH 0/5] cifs: Fix xid leak in cifs

Re: [PATCH 0/5] cifs: Fix xid leak in cifs

[Paulo Alcantara]

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>

[PATCH 0/5] cifs: Fix xid leak in cifs

[Zhang Xiaoxu]

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

[PATCH 1/5] cifs: Fix xid leak in cifs_create()

[Zhang Xiaoxu]

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

[PATCH 2/5] cifs: Fix xid leak in cifs_copy_file_range()

[Zhang Xiaoxu]

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

[PATCH 3/5] cifs: Fix xid leak in cifs_flock()

[Zhang Xiaoxu]

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

[PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel()

[Zhang Xiaoxu]

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

[PATCH 5/5] cifs: Fix xid leak in cifs_get_file_info_unix()

[Zhang Xiaoxu]

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

Re: [PATCH 0/5] cifs: Fix xid leak in cifs

[Steve French]

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

Re: [PATCH 4/5] cifs: Fix xid leak in cifs_ses_add_channel()

[Steve French]

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