| Message ID | 20240928215948.4494-3-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Fix support for NFS-style reparse points | expand |
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 >
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.
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; }
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(-)