Clean up some places where previously an extra element in the kvec array
was being used to hold an rfc1002 header for SMB1 (a previous patch removed
this and generated it on the fly as for SMB2/3).
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Steve French <sfrench@samba.org>
cc: Paulo Alcantara <pc@manguebit.org>
cc: Shyam Prasad N <sprasad@microsoft.com>
cc: Tom Talpey <tom@talpey.com>
cc: linux-cifs@vger.kernel.org
cc: netfs@lists.linux.dev
cc: linux-fsdevel@vger.kernel.org
---
fs/smb/client/cifsencrypt.c | 52 ++++++++---------------------------
fs/smb/client/cifsproto.h | 5 ----
fs/smb/client/cifstransport.c | 20 ++------------
fs/smb/client/smb1ops.c | 12 +++++---
fs/smb/client/transport.c | 25 +++++++++--------
5 files changed, 35 insertions(+), 79 deletions(-)
diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index 1e0ac87c6686..a9a57904c6b1 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -86,26 +86,21 @@ static int cifs_sig_iter(const struct iov_iter *iter, size_t maxsize,
int __cifs_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
char *signature, struct cifs_calc_sig_ctx *ctx)
{
- int i;
+ struct iov_iter iter;
ssize_t rc;
- struct kvec *iov = rqst->rq_iov;
- int n_vec = rqst->rq_nvec;
+ size_t size = 0;
- for (i = 0; i < n_vec; i++) {
- if (iov[i].iov_len == 0)
- continue;
- if (iov[i].iov_base == NULL) {
- cifs_dbg(VFS, "null iovec entry\n");
- return -EIO;
- }
+ for (int i = 0; i < rqst->rq_nvec; i++)
+ size += rqst->rq_iov[i].iov_len;
- rc = cifs_sig_update(ctx, iov[i].iov_base, iov[i].iov_len);
- if (rc) {
- cifs_dbg(VFS, "%s: Could not update with payload\n",
- __func__);
- return rc;
- }
- }
+ iov_iter_kvec(&iter, ITER_SOURCE, rqst->rq_iov, rqst->rq_nvec, size);
+
+ if (iov_iter_count(&iter) <= 4)
+ return -EIO;
+
+ rc = cifs_sig_iter(&iter, iov_iter_count(&iter), ctx);
+ if (rc < 0)
+ return rc;
rc = cifs_sig_iter(&rqst->rq_iter, iov_iter_count(&rqst->rq_iter), ctx);
if (rc < 0)
@@ -186,29 +181,6 @@ int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
return rc;
}
-int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
- __u32 *pexpected_response_sequence)
-{
- struct smb_rqst rqst = { .rq_iov = iov,
- .rq_nvec = n_vec };
-
- return cifs_sign_rqst(&rqst, server, pexpected_response_sequence);
-}
-
-/* must be called with server->srv_mutex held */
-int cifs_sign_smb(struct smb_hdr *cifs_pdu, unsigned int pdu_len,
- struct TCP_Server_Info *server,
- __u32 *pexpected_response_sequence_number)
-{
- struct kvec iov[1] = {
- [0].iov_base = (char *)cifs_pdu,
- [0].iov_len = pdu_len,
- };
-
- return cifs_sign_smbv(iov, ARRAY_SIZE(iov), server,
- pexpected_response_sequence_number);
-}
-
int cifs_verify_signature(struct smb_rqst *rqst,
struct TCP_Server_Info *server,
__u32 expected_sequence_number)
diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
index 4af6ea8150c3..39586f2dfad5 100644
--- a/fs/smb/client/cifsproto.h
+++ b/fs/smb/client/cifsproto.h
@@ -68,11 +68,6 @@ int __cifs_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
char *signature, struct cifs_calc_sig_ctx *ctx);
int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
__u32 *pexpected_response_sequence_number);
-int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
- __u32 *pexpected_response_sequence);
-int cifs_sign_smb(struct smb_hdr *cifs_pdu, unsigned int pdu_len,
- struct TCP_Server_Info *server,
- __u32 *pexpected_response_sequence_number);
int cifs_verify_signature(struct smb_rqst *rqst,
struct TCP_Server_Info *server,
__u32 expected_sequence_number);
diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c
index 1a0b80fc97d4..6fa60de786e9 100644
--- a/fs/smb/client/cifstransport.c
+++ b/fs/smb/client/cifstransport.c
@@ -71,22 +71,6 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
return smb;
}
-int
-smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
- unsigned int smb_buf_length)
-{
- struct kvec iov[1] = {
- [0].iov_base = smb_buffer,
- [0].iov_len = smb_buf_length,
- };
- struct smb_rqst rqst = {
- .rq_iov = iov,
- .rq_nvec = ARRAY_SIZE(iov),
- };
-
- return __smb_send_rqst(server, 1, &rqst);
-}
-
static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf,
struct smb_message **ppmidQ)
{
@@ -370,7 +354,7 @@ int SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
return rc;
}
- rc = cifs_sign_smb(in_buf, in_len, server, &smb->sequence_number);
+ rc = cifs_sign_rqst(&rqst, server, &smb->sequence_number);
if (rc) {
delete_mid(smb);
cifs_server_unlock(server);
@@ -378,7 +362,7 @@ int SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
}
smb->mid_state = MID_REQUEST_SUBMITTED;
- rc = smb_send(server, in_buf, in_len);
+ rc = __smb_send_rqst(server, 1, &rqst);
cifs_save_when_sent(smb);
if (rc < 0)
diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
index 91205685057c..6b0d0b511b9f 100644
--- a/fs/smb/client/smb1ops.c
+++ b/fs/smb/client/smb1ops.c
@@ -34,17 +34,21 @@ static int
send_nt_cancel(struct TCP_Server_Info *server, struct smb_rqst *rqst,
struct smb_message *smb)
{
- int rc = 0;
struct smb_hdr *in_buf = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
- unsigned int in_len = rqst->rq_iov[0].iov_len;
+ struct kvec iov[1];
+ struct smb_rqst crqst = { .rq_iov = iov, .rq_nvec = 1 };
+ int rc = 0;
/* +2 for BCC field */
in_buf->Command = SMB_COM_NT_CANCEL;
in_buf->WordCount = 0;
put_bcc(0, in_buf);
+ iov[0].iov_base = in_buf;
+ iov[0].iov_len = sizeof(struct smb_hdr) + 2;
+
cifs_server_lock(server);
- rc = cifs_sign_smb(in_buf, in_len, server, &smb->sequence_number);
+ rc = cifs_sign_rqst(&crqst, server, &smb->sequence_number);
if (rc) {
cifs_server_unlock(server);
return rc;
@@ -56,7 +60,7 @@ send_nt_cancel(struct TCP_Server_Info *server, struct smb_rqst *rqst,
* after signing here.
*/
--server->sequence_number;
- rc = smb_send(server, in_buf, in_len);
+ rc = __smb_send_rqst(server, 1, &crqst);
if (rc < 0)
server->sequence_number--;
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 2e338e186809..c023c9873c88 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -1043,22 +1043,23 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
goto out;
}
- buf = (char *)smb[i]->resp_buf;
- resp_iov[i].iov_base = buf;
- resp_iov[i].iov_len = smb[i]->resp_buf_size;
-
- if (smb[i]->large_buf)
- resp_buf_type[i] = CIFS_LARGE_BUFFER;
- else
- resp_buf_type[i] = CIFS_SMALL_BUFFER;
-
rc = server->ops->check_receive(smb[i], server,
flags & CIFS_LOG_ERROR);
- /* mark it so buf will not be freed by delete_mid */
- if ((flags & CIFS_NO_RSP_BUF) == 0)
- smb[i]->resp_buf = NULL;
+ if (resp_iov) {
+ buf = (char *)smb[i]->resp_buf;
+ resp_iov[i].iov_base = buf;
+ resp_iov[i].iov_len = smb[i]->resp_buf_size;
+ if (smb[i]->large_buf)
+ resp_buf_type[i] = CIFS_LARGE_BUFFER;
+ else
+ resp_buf_type[i] = CIFS_SMALL_BUFFER;
+
+ /* mark it so buf will not be freed by delete_mid */
+ if ((flags & CIFS_NO_RSP_BUF) == 0)
+ smb[i]->resp_buf = NULL;
+ }
}
/*Hi David,
I had to squash this into the patch
I'm using smatch when building and got the following error
with this change:
client/transport.c:1073 compound_send_recv() error: we previously assumed 'resp_iov' could be null (see line 1051)
See my inline comment below to see where the problem is introduced.
It goes away when I squash the following:
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index c023c9873c88..7036acd6542b 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -995,6 +995,9 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
if ((ses->ses_status == SES_NEW) || (optype & CIFS_NEG_OP) || (optype & CIFS_SESS_OP)) {
spin_unlock(&ses->ses_lock);
+ if (WARN_ON_ONCE(num_rqst != 1 || !resp_iov))
+ return -EINVAL;
+
cifs_server_lock(server);
smb311_update_preauth_hash(ses, server, rqst[0].rq_iov, rqst[0].rq_nvec);
cifs_server_unlock(server);
Am 24.11.25 um 13:42 schrieb David Howells:
> Clean up some places where previously an extra element in the kvec array
> was being used to hold an rfc1002 header for SMB1 (a previous patch removed
> this and generated it on the fly as for SMB2/3).
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Steve French <sfrench@samba.org>
> cc: Paulo Alcantara <pc@manguebit.org>
> cc: Shyam Prasad N <sprasad@microsoft.com>
> cc: Tom Talpey <tom@talpey.com>
> cc: linux-cifs@vger.kernel.org
> cc: netfs@lists.linux.dev
> cc: linux-fsdevel@vger.kernel.org
> ---
> fs/smb/client/cifsencrypt.c | 52 ++++++++---------------------------
> fs/smb/client/cifsproto.h | 5 ----
> fs/smb/client/cifstransport.c | 20 ++------------
> fs/smb/client/smb1ops.c | 12 +++++---
> fs/smb/client/transport.c | 25 +++++++++--------
> 5 files changed, 35 insertions(+), 79 deletions(-)
>
> diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
> index 1e0ac87c6686..a9a57904c6b1 100644
> --- a/fs/smb/client/cifsencrypt.c
> +++ b/fs/smb/client/cifsencrypt.c
> @@ -86,26 +86,21 @@ static int cifs_sig_iter(const struct iov_iter *iter, size_t maxsize,
> int __cifs_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
> char *signature, struct cifs_calc_sig_ctx *ctx)
> {
> - int i;
> + struct iov_iter iter;
> ssize_t rc;
> - struct kvec *iov = rqst->rq_iov;
> - int n_vec = rqst->rq_nvec;
> + size_t size = 0;
>
> - for (i = 0; i < n_vec; i++) {
> - if (iov[i].iov_len == 0)
> - continue;
> - if (iov[i].iov_base == NULL) {
> - cifs_dbg(VFS, "null iovec entry\n");
> - return -EIO;
> - }
> + for (int i = 0; i < rqst->rq_nvec; i++)
> + size += rqst->rq_iov[i].iov_len;
>
> - rc = cifs_sig_update(ctx, iov[i].iov_base, iov[i].iov_len);
> - if (rc) {
> - cifs_dbg(VFS, "%s: Could not update with payload\n",
> - __func__);
> - return rc;
> - }
> - }
> + iov_iter_kvec(&iter, ITER_SOURCE, rqst->rq_iov, rqst->rq_nvec, size);
> +
> + if (iov_iter_count(&iter) <= 4)
> + return -EIO;
> +
> + rc = cifs_sig_iter(&iter, iov_iter_count(&iter), ctx);
> + if (rc < 0)
> + return rc;
>
> rc = cifs_sig_iter(&rqst->rq_iter, iov_iter_count(&rqst->rq_iter), ctx);
> if (rc < 0)
> @@ -186,29 +181,6 @@ int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
> return rc;
> }
>
> -int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
> - __u32 *pexpected_response_sequence)
> -{
> - struct smb_rqst rqst = { .rq_iov = iov,
> - .rq_nvec = n_vec };
> -
> - return cifs_sign_rqst(&rqst, server, pexpected_response_sequence);
> -}
> -
> -/* must be called with server->srv_mutex held */
> -int cifs_sign_smb(struct smb_hdr *cifs_pdu, unsigned int pdu_len,
> - struct TCP_Server_Info *server,
> - __u32 *pexpected_response_sequence_number)
> -{
> - struct kvec iov[1] = {
> - [0].iov_base = (char *)cifs_pdu,
> - [0].iov_len = pdu_len,
> - };
> -
> - return cifs_sign_smbv(iov, ARRAY_SIZE(iov), server,
> - pexpected_response_sequence_number);
> -}
> -
> int cifs_verify_signature(struct smb_rqst *rqst,
> struct TCP_Server_Info *server,
> __u32 expected_sequence_number)
> diff --git a/fs/smb/client/cifsproto.h b/fs/smb/client/cifsproto.h
> index 4af6ea8150c3..39586f2dfad5 100644
> --- a/fs/smb/client/cifsproto.h
> +++ b/fs/smb/client/cifsproto.h
> @@ -68,11 +68,6 @@ int __cifs_calc_signature(struct smb_rqst *rqst, struct TCP_Server_Info *server,
> char *signature, struct cifs_calc_sig_ctx *ctx);
> int cifs_sign_rqst(struct smb_rqst *rqst, struct TCP_Server_Info *server,
> __u32 *pexpected_response_sequence_number);
> -int cifs_sign_smbv(struct kvec *iov, int n_vec, struct TCP_Server_Info *server,
> - __u32 *pexpected_response_sequence);
> -int cifs_sign_smb(struct smb_hdr *cifs_pdu, unsigned int pdu_len,
> - struct TCP_Server_Info *server,
> - __u32 *pexpected_response_sequence_number);
> int cifs_verify_signature(struct smb_rqst *rqst,
> struct TCP_Server_Info *server,
> __u32 expected_sequence_number);
> diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c
> index 1a0b80fc97d4..6fa60de786e9 100644
> --- a/fs/smb/client/cifstransport.c
> +++ b/fs/smb/client/cifstransport.c
> @@ -71,22 +71,6 @@ alloc_mid(const struct smb_hdr *smb_buffer, struct TCP_Server_Info *server)
> return smb;
> }
>
> -int
> -smb_send(struct TCP_Server_Info *server, struct smb_hdr *smb_buffer,
> - unsigned int smb_buf_length)
> -{
> - struct kvec iov[1] = {
> - [0].iov_base = smb_buffer,
> - [0].iov_len = smb_buf_length,
> - };
> - struct smb_rqst rqst = {
> - .rq_iov = iov,
> - .rq_nvec = ARRAY_SIZE(iov),
> - };
> -
> - return __smb_send_rqst(server, 1, &rqst);
> -}
> -
> static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf,
> struct smb_message **ppmidQ)
> {
> @@ -370,7 +354,7 @@ int SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
> return rc;
> }
>
> - rc = cifs_sign_smb(in_buf, in_len, server, &smb->sequence_number);
> + rc = cifs_sign_rqst(&rqst, server, &smb->sequence_number);
> if (rc) {
> delete_mid(smb);
> cifs_server_unlock(server);
> @@ -378,7 +362,7 @@ int SendReceiveBlockingLock(const unsigned int xid, struct cifs_tcon *tcon,
> }
>
> smb->mid_state = MID_REQUEST_SUBMITTED;
> - rc = smb_send(server, in_buf, in_len);
> + rc = __smb_send_rqst(server, 1, &rqst);
> cifs_save_when_sent(smb);
>
> if (rc < 0)
> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
> index 91205685057c..6b0d0b511b9f 100644
> --- a/fs/smb/client/smb1ops.c
> +++ b/fs/smb/client/smb1ops.c
> @@ -34,17 +34,21 @@ static int
> send_nt_cancel(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> struct smb_message *smb)
> {
> - int rc = 0;
> struct smb_hdr *in_buf = (struct smb_hdr *)rqst->rq_iov[0].iov_base;
> - unsigned int in_len = rqst->rq_iov[0].iov_len;
> + struct kvec iov[1];
> + struct smb_rqst crqst = { .rq_iov = iov, .rq_nvec = 1 };
> + int rc = 0;
>
> /* +2 for BCC field */
> in_buf->Command = SMB_COM_NT_CANCEL;
> in_buf->WordCount = 0;
> put_bcc(0, in_buf);
>
> + iov[0].iov_base = in_buf;
> + iov[0].iov_len = sizeof(struct smb_hdr) + 2;
> +
> cifs_server_lock(server);
> - rc = cifs_sign_smb(in_buf, in_len, server, &smb->sequence_number);
> + rc = cifs_sign_rqst(&crqst, server, &smb->sequence_number);
> if (rc) {
> cifs_server_unlock(server);
> return rc;
> @@ -56,7 +60,7 @@ send_nt_cancel(struct TCP_Server_Info *server, struct smb_rqst *rqst,
> * after signing here.
> */
> --server->sequence_number;
> - rc = smb_send(server, in_buf, in_len);
> + rc = __smb_send_rqst(server, 1, &crqst);
> if (rc < 0)
> server->sequence_number--;
>
> diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
> index 2e338e186809..c023c9873c88 100644
> --- a/fs/smb/client/transport.c
> +++ b/fs/smb/client/transport.c
> @@ -1043,22 +1043,23 @@ compound_send_recv(const unsigned int xid, struct cifs_ses *ses,
> goto out;
> }
>
> - buf = (char *)smb[i]->resp_buf;
> - resp_iov[i].iov_base = buf;
> - resp_iov[i].iov_len = smb[i]->resp_buf_size;
> -
> - if (smb[i]->large_buf)
> - resp_buf_type[i] = CIFS_LARGE_BUFFER;
> - else
> - resp_buf_type[i] = CIFS_SMALL_BUFFER;
> -
> rc = server->ops->check_receive(smb[i], server,
> flags & CIFS_LOG_ERROR);
>
> - /* mark it so buf will not be freed by delete_mid */
> - if ((flags & CIFS_NO_RSP_BUF) == 0)
> - smb[i]->resp_buf = NULL;
> + if (resp_iov) {
Is it really possible that resp_iov is NULL here?
I guess it is for things with CIFS_NO_RSP_BUF, correct?
> + buf = (char *)smb[i]->resp_buf;
> + resp_iov[i].iov_base = buf;
> + resp_iov[i].iov_len = smb[i]->resp_buf_size;
>
> + if (smb[i]->large_buf)
> + resp_buf_type[i] = CIFS_LARGE_BUFFER;
> + else
> + resp_buf_type[i] = CIFS_SMALL_BUFFER;
> +
> + /* mark it so buf will not be freed by delete_mid */
> + if ((flags & CIFS_NO_RSP_BUF) == 0)
> + smb[i]->resp_buf = NULL;
> + }
> }
>
> /*
>
Hi Stefan,
Stefan Metzmacher <metze@samba.org> wrote:
> I had to squash this into the patch
>
> I'm using smatch when building and got the following error
> with this change:
>
> client/transport.c:1073 compound_send_recv() error: we previously assumed 'resp_iov' could be null (see line 1051)
> ...
> if ((ses->ses_status == SES_NEW) || (optype & CIFS_NEG_OP) || (optype & CIFS_SESS_OP)) {
In this case smatch is wrong, though it can't work this out as the context
spans more than one file. This clause applies only to certain operations
(such as session setup and negotiate) that will always have a response buffer.
But I've no objection to adding this warning to splat the warning.
> > + if (resp_iov) {
>
> Is it really possible that resp_iov is NULL here?
>
> I guess it is for things with CIFS_NO_RSP_BUF, correct?
It probably can't be NULL; even those things that provide CIFS_NO_RSP_BUF seem
to pass a resp_iov, though they tend not to actually initialise it.
Hopefully, this will go away in future and stuff will be attached to the
smb_message struct which will be allocated and passed down from the likes of
smb2pdu.c or cifssmb.c to transport.c.
David
Am 24.11.25 um 15:36 schrieb David Howells:
> Hi Stefan,
>
> Stefan Metzmacher <metze@samba.org> wrote:
>
>> I had to squash this into the patch
>>
>> I'm using smatch when building and got the following error
>> with this change:
>>
>> client/transport.c:1073 compound_send_recv() error: we previously assumed 'resp_iov' could be null (see line 1051)
>> ...
>> if ((ses->ses_status == SES_NEW) || (optype & CIFS_NEG_OP) || (optype & CIFS_SESS_OP)) {
>
> In this case smatch is wrong, though it can't work this out as the context
> spans more than one file. This clause applies only to certain operations
> (such as session setup and negotiate) that will always have a response buffer.
> But I've no objection to adding this warning to splat the warning.
Ok, I can just squash as well as the EIO changes below my branch
I'll hopefully be able to post later today or tomorrow.
My idea would be that my branch would replace ksmbd-for-next
and add your any my changes on top.
Thanks!
metze
Stefan Metzmacher <metze@samba.org> wrote: > Ok, I can just squash as well as the EIO changes below my branch > I'll hopefully be able to post later today or tomorrow. > > My idea would be that my branch would replace ksmbd-for-next > and add your any my changes on top. I've merged in your requested changes and repushed my branch. David
Am 24.11.25 um 15:44 schrieb David Howells: > Stefan Metzmacher <metze@samba.org> wrote: > >> Ok, I can just squash as well as the EIO changes below my branch >> I'll hopefully be able to post later today or tomorrow. >> >> My idea would be that my branch would replace ksmbd-for-next >> and add your any my changes on top. > > I've merged in your requested changes and repushed my branch. Great thanks! metze
Hi Metze, Do you want me to repost my patches so you can associate URLs with them? David
Hi David, > Do you want me to repost my patches so you can associate URLs with them? I don't need that, I'm just rebasing on your branch now. I don't know what Steve needs in order to put it into ksmbd-for-next and what review tags from Tom he should add. Steve feel free to add 'Acked-by: Stefan Metzmacher <metze@samba.org>' Thanks! metze
© 2016 - 2025 Red Hat, Inc.