If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
intermediate object created by CREATE. Otherwise empty object stay on the
server when reparse call failed.

This ensures that if the creating of special files is unsupported by the
server then no empty file stay on the server as a result of unsupported
operation.

Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 fs/smb/client/smb2inode.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 11a1c53c64e0..af42f44bdcf4 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -1205,6 +1205,8 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
 	struct cifsFileInfo *cfile;
 	struct inode *new = NULL;
+	int out_buftype[2] = {};
+	struct kvec out_iov[2];
 	struct kvec in_iov[2];
 	int cmds[2];
 	int rc;
@@ -1228,7 +1230,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = smb311_posix_get_inode_info(&new, full_path,
 							 data, sb, xid);
@@ -1237,12 +1239,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = cifs_get_inode_info(&new, full_path,
 						 data, sb, xid, NULL);
 		}
 	}
+
+	if (rc) {
+		/*
+		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
+		 * remove the intermediate object created by CREATE. Otherwise
+		 * empty object stay on the server when reparse call failed.
+		 */
+		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
+		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
+			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
+	}
+
+	free_rsp_buf(out_buftype[0], out_iov[0].iov_base);
+	free_rsp_buf(out_buftype[1], out_iov[1].iov_base);
+
 	return rc ? ERR_PTR(rc) : new;
 }
 
-- 
2.20.1

Pali Rohár <pali@kernel.org> writes:

> If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
> intermediate object created by CREATE. Otherwise empty object stay on the
> server when reparse call failed.
>
> This ensures that if the creating of special files is unsupported by the
> server then no empty file stay on the server as a result of unsupported
> operation.
>
> Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/smb2inode.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 11a1c53c64e0..af42f44bdcf4 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -1205,6 +1205,8 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>  	struct cifsFileInfo *cfile;
>  	struct inode *new = NULL;
> +	int out_buftype[2] = {};
> +	struct kvec out_iov[2];
>  	struct kvec in_iov[2];
>  	int cmds[2];
>  	int rc;
> @@ -1228,7 +1230,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
>  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>  		if (!rc) {
>  			rc = smb311_posix_get_inode_info(&new, full_path,
>  							 data, sb, xid);
> @@ -1237,12 +1239,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  		cmds[1] = SMB2_OP_QUERY_INFO;
>  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>  		if (!rc) {
>  			rc = cifs_get_inode_info(&new, full_path,
>  						 data, sb, xid, NULL);
>  		}
>  	}
> +
> +	if (rc) {
> +		/*
> +		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
> +		 * remove the intermediate object created by CREATE. Otherwise
> +		 * empty object stay on the server when reparse call failed.
> +		 */
> +		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
> +		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
> +			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
> +	}

You should handle the case where ->iov_base is NULL or out_buftype ==
CIFS_NO_BUFFER, otherwise you'll end up with a NULL ptr deref.

On Monday 30 September 2024 12:25:27 Paulo Alcantara wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
> > intermediate object created by CREATE. Otherwise empty object stay on the
> > server when reparse call failed.
> >
> > This ensures that if the creating of special files is unsupported by the
> > server then no empty file stay on the server as a result of unsupported
> > operation.
> >
> > Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  fs/smb/client/smb2inode.c | 21 +++++++++++++++++++--
> >  1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> > index 11a1c53c64e0..af42f44bdcf4 100644
> > --- a/fs/smb/client/smb2inode.c
> > +++ b/fs/smb/client/smb2inode.c
> > @@ -1205,6 +1205,8 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> >  	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
> >  	struct cifsFileInfo *cfile;
> >  	struct inode *new = NULL;
> > +	int out_buftype[2] = {};
> > +	struct kvec out_iov[2];
> >  	struct kvec in_iov[2];
> >  	int cmds[2];
> >  	int rc;
> > @@ -1228,7 +1230,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> >  		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
> >  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> >  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> > -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> > +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
> >  		if (!rc) {
> >  			rc = smb311_posix_get_inode_info(&new, full_path,
> >  							 data, sb, xid);
> > @@ -1237,12 +1239,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
> >  		cmds[1] = SMB2_OP_QUERY_INFO;
> >  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
> >  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> > -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> > +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
> >  		if (!rc) {
> >  			rc = cifs_get_inode_info(&new, full_path,
> >  						 data, sb, xid, NULL);
> >  		}
> >  	}
> > +
> > +	if (rc) {
> > +		/*
> > +		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
> > +		 * remove the intermediate object created by CREATE. Otherwise
> > +		 * empty object stay on the server when reparse call failed.
> > +		 */
> > +		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
> > +		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
> > +			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
> > +	}
> 
> You should handle the case where ->iov_base is NULL or out_buftype ==
> CIFS_NO_BUFFER, otherwise you'll end up with a NULL ptr deref.

Ok, thanks for info! I will send v3 with those checks.

Anyway, what does it mean if iov_base stay NULL or out_buftype is
CIFS_NO_BUFFER? Does it mean that the server has not returned reply for
that command?

I guess that it should be treated as unsuccessful result.

Pali Rohár <pali@kernel.org> writes:

> On Monday 30 September 2024 12:25:27 Paulo Alcantara wrote:
>> Pali Rohár <pali@kernel.org> writes:
>> 
>> > If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
>> > intermediate object created by CREATE. Otherwise empty object stay on the
>> > server when reparse call failed.
>> >
>> > This ensures that if the creating of special files is unsupported by the
>> > server then no empty file stay on the server as a result of unsupported
>> > operation.
>> >
>> > Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
>> > Signed-off-by: Pali Rohár <pali@kernel.org>
>> > ---
>> >  fs/smb/client/smb2inode.c | 21 +++++++++++++++++++--
>> >  1 file changed, 19 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
>> > index 11a1c53c64e0..af42f44bdcf4 100644
>> > --- a/fs/smb/client/smb2inode.c
>> > +++ b/fs/smb/client/smb2inode.c
>> > @@ -1205,6 +1205,8 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>> >  	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>> >  	struct cifsFileInfo *cfile;
>> >  	struct inode *new = NULL;
>> > +	int out_buftype[2] = {};
>> > +	struct kvec out_iov[2];
>> >  	struct kvec in_iov[2];
>> >  	int cmds[2];
>> >  	int rc;
>> > @@ -1228,7 +1230,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>> >  		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
>> >  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>> >  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
>> > -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
>> > +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>> >  		if (!rc) {
>> >  			rc = smb311_posix_get_inode_info(&new, full_path,
>> >  							 data, sb, xid);
>> > @@ -1237,12 +1239,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>> >  		cmds[1] = SMB2_OP_QUERY_INFO;
>> >  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>> >  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
>> > -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
>> > +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>> >  		if (!rc) {
>> >  			rc = cifs_get_inode_info(&new, full_path,
>> >  						 data, sb, xid, NULL);
>> >  		}
>> >  	}
>> > +
>> > +	if (rc) {
>> > +		/*
>> > +		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
>> > +		 * remove the intermediate object created by CREATE. Otherwise
>> > +		 * empty object stay on the server when reparse call failed.
>> > +		 */
>> > +		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
>> > +		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
>> > +			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
>> > +	}
>> 
>> You should handle the case where ->iov_base is NULL or out_buftype ==
>> CIFS_NO_BUFFER, otherwise you'll end up with a NULL ptr deref.
>
> Ok, thanks for info! I will send v3 with those checks.
>
> Anyway, what does it mean if iov_base stay NULL or out_buftype is
> CIFS_NO_BUFFER? Does it mean that the server has not returned reply for
> that command?

Not necessarily.  Just consider a simple case where smb2_compound_op()
would fail to allocate memory for @vars, then it would return -ENOMEM
and you would end up dereferencing ->iov_base which is still NULL.  That
is, compound_send_recv() wasn't called and then no response buffers were
set yet.

Hello Steve, I was reading again implementation of smb2_compound_op()
function and if I understood it correctly, then out_iov and out_buftype
array parameters needs to have num_cmds+2 members. This is quite
unexpected, and this patch cause buffer overflow and memory leaks.
+2 is for CREATE and CLOSE operations added automatically by compound.

Surprisingly, no memory issue and neither corrupted packed I observed.
And therefore I thought that change is working fine.

I will send a new version of this change to increase array members and
free all members of array.

On Saturday 28 September 2024 23:59:42 Pali Rohár wrote:
> If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
> intermediate object created by CREATE. Otherwise empty object stay on the
> server when reparse call failed.
> 
> This ensures that if the creating of special files is unsupported by the
> server then no empty file stay on the server as a result of unsupported
> operation.
> 
> Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  fs/smb/client/smb2inode.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 11a1c53c64e0..af42f44bdcf4 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -1205,6 +1205,8 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>  	struct cifsFileInfo *cfile;
>  	struct inode *new = NULL;
> +	int out_buftype[2] = {};
> +	struct kvec out_iov[2];
>  	struct kvec in_iov[2];
>  	int cmds[2];
>  	int rc;
> @@ -1228,7 +1230,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
>  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>  		if (!rc) {
>  			rc = smb311_posix_get_inode_info(&new, full_path,
>  							 data, sb, xid);
> @@ -1237,12 +1239,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>  		cmds[1] = SMB2_OP_QUERY_INFO;
>  		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>  		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>  		if (!rc) {
>  			rc = cifs_get_inode_info(&new, full_path,
>  						 data, sb, xid, NULL);
>  		}
>  	}
> +
> +	if (rc) {
> +		/*
> +		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
> +		 * remove the intermediate object created by CREATE. Otherwise
> +		 * empty object stay on the server when reparse call failed.
> +		 */
> +		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
> +		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
> +			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
> +	}
> +
> +	free_rsp_buf(out_buftype[0], out_iov[0].iov_base);
> +	free_rsp_buf(out_buftype[1], out_iov[1].iov_base);
> +
>  	return rc ? ERR_PTR(rc) : new;
>  }
>  
> -- 
> 2.20.1
> 

If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
intermediate object created by CREATE. Otherwise empty object stay on the
server when reparse call failed.

This ensures that if the creating of special files is unsupported by the
server then no empty file stay on the server as a result of unsupported
operation.

Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v3:
* Check if iov_base and out_buftype are valid before derefrencing iov_base
Changes in v2:
* Increase out_buftype[] and out_iov[] members from 2 to 4 as required by smb2_compound_op
* Call free_rsp_buf() for all members of out_buftype[]/out_iov[]
---
 fs/smb/client/smb2inode.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 11a1c53c64e0..a6dab60e2c01 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -1205,9 +1205,12 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
 	struct cifsFileInfo *cfile;
 	struct inode *new = NULL;
+	int out_buftype[4] = {};
+	struct kvec out_iov[4] = {};
 	struct kvec in_iov[2];
 	int cmds[2];
 	int rc;
+	int i;
 
 	oparms = CIFS_OPARMS(cifs_sb, tcon, full_path,
 			     SYNCHRONIZE | DELETE |
@@ -1228,7 +1231,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = smb311_posix_get_inode_info(&new, full_path,
 							 data, sb, xid);
@@ -1237,12 +1240,29 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = cifs_get_inode_info(&new, full_path,
 						 data, sb, xid, NULL);
 		}
 	}
+
+
+	/*
+	 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
+	 * remove the intermediate object created by CREATE. Otherwise
+	 * empty object stay on the server when reparse call failed.
+	 */
+	if (rc &&
+	    out_iov[0].iov_base != NULL && out_buftype[0] != CIFS_NO_BUFFER &&
+	    ((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
+	    (out_iov[1].iov_base == NULL || out_buftype[1] == CIFS_NO_BUFFER ||
+	     ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS))
+		smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
+
+	for (i = 0; i < ARRAY_SIZE(out_buftype); i++)
+		free_rsp_buf(out_buftype[i], out_iov[i].iov_base);
+
 	return rc ? ERR_PTR(rc) : new;
 }
 
-- 
2.20.1

tentatively merged into cifs-2.6.git pending review and testing

On Mon, Sep 30, 2024 at 3:28 PM Pali Rohár <pali@kernel.org> wrote:
>
> If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
> intermediate object created by CREATE. Otherwise empty object stay on the
> server when reparse call failed.
>
> This ensures that if the creating of special files is unsupported by the
> server then no empty file stay on the server as a result of unsupported
> operation.
>
> Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v3:
> * Check if iov_base and out_buftype are valid before derefrencing iov_base
> Changes in v2:
> * Increase out_buftype[] and out_iov[] members from 2 to 4 as required by smb2_compound_op
> * Call free_rsp_buf() for all members of out_buftype[]/out_iov[]
> ---
>  fs/smb/client/smb2inode.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 11a1c53c64e0..a6dab60e2c01 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -1205,9 +1205,12 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>         struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>         struct cifsFileInfo *cfile;
>         struct inode *new = NULL;
> +       int out_buftype[4] = {};
> +       struct kvec out_iov[4] = {};
>         struct kvec in_iov[2];
>         int cmds[2];
>         int rc;
> +       int i;
>
>         oparms = CIFS_OPARMS(cifs_sb, tcon, full_path,
>                              SYNCHRONIZE | DELETE |
> @@ -1228,7 +1231,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -                                     in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +                                     in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>                 if (!rc) {
>                         rc = smb311_posix_get_inode_info(&new, full_path,
>                                                          data, sb, xid);
> @@ -1237,12 +1240,29 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cmds[1] = SMB2_OP_QUERY_INFO;
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -                                     in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +                                     in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>                 if (!rc) {
>                         rc = cifs_get_inode_info(&new, full_path,
>                                                  data, sb, xid, NULL);
>                 }
>         }
> +
> +
> +       /*
> +        * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
> +        * remove the intermediate object created by CREATE. Otherwise
> +        * empty object stay on the server when reparse call failed.
> +        */
> +       if (rc &&
> +           out_iov[0].iov_base != NULL && out_buftype[0] != CIFS_NO_BUFFER &&
> +           ((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
> +           (out_iov[1].iov_base == NULL || out_buftype[1] == CIFS_NO_BUFFER ||
> +            ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS))
> +               smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
> +
> +       for (i = 0; i < ARRAY_SIZE(out_buftype); i++)
> +               free_rsp_buf(out_buftype[i], out_iov[i].iov_base);
> +
>         return rc ? ERR_PTR(rc) : new;
>  }
>
> --
> 2.20.1
>
>


-- 
Thanks,

Steve

If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
intermediate object created by CREATE. Otherwise empty object stay on the
server when reparse call failed.

This ensures that if the creating of special files is unsupported by the
server then no empty file stay on the server as a result of unsupported
operation.

Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
Changes in v2:
* Increase out_buftype[] and out_iov[] members from 2 to 4 as required by smb2_compound_op
* Call free_rsp_buf() for all members of out_buftype[]/out_iov[]

I would like if you double check this smb2_compound_op() usage if there
is not some other memory issue. As V1 contained both memory leak and
buffer overflow (smb2_compound_op wrote out of those two arrays).

---
 fs/smb/client/smb2inode.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 11a1c53c64e0..6e69a3b98be3 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -1205,9 +1205,12 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
 	struct cifsFileInfo *cfile;
 	struct inode *new = NULL;
+	int out_buftype[4] = {};
+	struct kvec out_iov[4] = {};
 	struct kvec in_iov[2];
 	int cmds[2];
 	int rc;
+	int i;
 
 	oparms = CIFS_OPARMS(cifs_sb, tcon, full_path,
 			     SYNCHRONIZE | DELETE |
@@ -1228,7 +1231,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = smb311_posix_get_inode_info(&new, full_path,
 							 data, sb, xid);
@@ -1237,12 +1240,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
 		cmds[1] = SMB2_OP_QUERY_INFO;
 		cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
 		rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
-				      in_iov, cmds, 2, cfile, NULL, NULL, NULL);
+				      in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
 		if (!rc) {
 			rc = cifs_get_inode_info(&new, full_path,
 						 data, sb, xid, NULL);
 		}
 	}
+
+	if (rc) {
+		/*
+		 * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
+		 * remove the intermediate object created by CREATE. Otherwise
+		 * empty object stay on the server when reparse call failed.
+		 */
+		if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
+		    ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
+			smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
+	}
+
+	for (i = 0; i < ARRAY_SIZE(out_buftype); i++)
+		free_rsp_buf(out_buftype[i], out_iov[i].iov_base);
+
 	return rc ? ERR_PTR(rc) : new;
 }
 
-- 
2.20.1

Running regression tests on these currently.  Let me know if
additional patches to add for these experiments

1c2fcb28ce99 (HEAD -> for-next, origin/for-next, origin/HEAD) cifs: Do
not convert delimiter when parsing NFS-style symlinks
92484193d70a cifs: Validate content of NFS reparse point buffer
c77a8e49f2d3 cifs: Fix buffer overflow when parsing NFS reparse points
ab7d68fd4bcc cifs: Remove intermediate object of failed create reparse call
ab50485ea1b4 smb: Update comments about some reparse point tags
1600fe2d42a1 smb: client: stop flooding dmesg with automounts
f7a33d56e52f smb: client: stop flooding dmesg on failed session setups
e1b72ef3ba03 cifs: Check for UTF-16 null codepoint in SFU symlink
target location
9717d5343849 Merge tag 'v6.12-rc-ksmbd-server-fixes' of
git://git.samba.org/ksmbd

On Sun, Sep 29, 2024 at 9:04 AM Pali Rohár <pali@kernel.org> wrote:
>
> If CREATE was successful but SMB2_OP_SET_REPARSE failed then remove the
> intermediate object created by CREATE. Otherwise empty object stay on the
> server when reparse call failed.
>
> This ensures that if the creating of special files is unsupported by the
> server then no empty file stay on the server as a result of unsupported
> operation.
>
> Fixes: 102466f303ff ("smb: client: allow creating special files via reparse points")
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
> Changes in v2:
> * Increase out_buftype[] and out_iov[] members from 2 to 4 as required by smb2_compound_op
> * Call free_rsp_buf() for all members of out_buftype[]/out_iov[]
>
> I would like if you double check this smb2_compound_op() usage if there
> is not some other memory issue. As V1 contained both memory leak and
> buffer overflow (smb2_compound_op wrote out of those two arrays).
>
> ---
>  fs/smb/client/smb2inode.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
> index 11a1c53c64e0..6e69a3b98be3 100644
> --- a/fs/smb/client/smb2inode.c
> +++ b/fs/smb/client/smb2inode.c
> @@ -1205,9 +1205,12 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>         struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
>         struct cifsFileInfo *cfile;
>         struct inode *new = NULL;
> +       int out_buftype[4] = {};
> +       struct kvec out_iov[4] = {};
>         struct kvec in_iov[2];
>         int cmds[2];
>         int rc;
> +       int i;
>
>         oparms = CIFS_OPARMS(cifs_sb, tcon, full_path,
>                              SYNCHRONIZE | DELETE |
> @@ -1228,7 +1231,7 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cmds[1] = SMB2_OP_POSIX_QUERY_INFO;
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -                                     in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +                                     in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>                 if (!rc) {
>                         rc = smb311_posix_get_inode_info(&new, full_path,
>                                                          data, sb, xid);
> @@ -1237,12 +1240,27 @@ struct inode *smb2_get_reparse_inode(struct cifs_open_info_data *data,
>                 cmds[1] = SMB2_OP_QUERY_INFO;
>                 cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile);
>                 rc = smb2_compound_op(xid, tcon, cifs_sb, full_path, &oparms,
> -                                     in_iov, cmds, 2, cfile, NULL, NULL, NULL);
> +                                     in_iov, cmds, 2, cfile, out_iov, out_buftype, NULL);
>                 if (!rc) {
>                         rc = cifs_get_inode_info(&new, full_path,
>                                                  data, sb, xid, NULL);
>                 }
>         }
> +
> +       if (rc) {
> +               /*
> +                * If CREATE was successful but SMB2_OP_SET_REPARSE failed then
> +                * remove the intermediate object created by CREATE. Otherwise
> +                * empty object stay on the server when reparse call failed.
> +                */
> +               if (((struct smb2_hdr *)out_iov[0].iov_base)->Status == STATUS_SUCCESS &&
> +                   ((struct smb2_hdr *)out_iov[1].iov_base)->Status != STATUS_SUCCESS)
> +                       smb2_unlink(xid, tcon, full_path, cifs_sb, NULL);
> +       }
> +
> +       for (i = 0; i < ARRAY_SIZE(out_buftype); i++)
> +               free_rsp_buf(out_buftype[i], out_iov[i].iov_base);
> +
>         return rc ? ERR_PTR(rc) : new;
>  }
>
> --
> 2.20.1
>
>


-- 
Thanks,

Steve