On 9/22/2022 6:32 PM, Enzo Matsumiya wrote:
Hi all,
This patch is the (apparent) correct way to fix the issues regarding some
messages with invalid signatures. My previous patch
https://lore.kernel.org/linux-cifs/20220918235442.2981-1-ematsumiya@xxxxxxx/
was wrong because a) it covered up the real issue (*), and b) I could never
again "reproduce" the "race" -- I had other patches in place when I tested it,
and (thus?) the system memory was not in "pristine" conditions, so it's very
likely that there's no race at all.
(*) Thanks a lot to Tom Talpey for pointing me to the right direction here.
Since it sucks to be wrong twice, I'm sending this one as RFC because I
wanted to confirm some things:
1) Can I rely on the fact that status != STATUS_SUCCESS means no variable
data in the message? I could only infer from the spec, but not really
confirm.
Technically, I think the answer is "no" but practically speaking maybe
"yes".
Not all errors are actually errors however, e.g. STOPPED_ON_SYMLINK and
the additional error contexts which accompany it. We definitely don't
want to skip validation of those.
There's also STATUS_PENDING, but that's special because it isn't actually
a response, it's just a "hang on I'm busy" that carries no other
data.
Finally, MS-SMB2 lists a few others in section 2.2.2.2, all of which
need validation I think.
2) (probably only in async cases) When the signature fails, for whatever
reason, we don't take any action. This doesn't seem right IMHO,
e.g. if the message is spoofed, we show a warning that the signatures
doesn't match, but I would expect at least for the operation to stop,
or maybe even a complete dis/reconnect.
I don't think so. The spec calls for dropping the message, and after
all it could have been simple packet corruption. The retries will sort
it out.
Absolutely positively do not print a message for each received packet,
good or bad.
3) For SMB1, I couldn't really use generic/465 as a real confirmation for
this because it says "O_DIRECT is not supported". From reading the
code and 'man mount.cifs' I understood that this is supported, so what
gives? Worth noting that I don't follow/use/test SMB1 too much.
The patch does work for other cases I tried though.
Honestly, I don't think we care. No amount of patching can possibly
save SMB1.
Tom.
I hope someone can address my questions/concerts above, and please let me
know if the patch is technically and semantically correct.
Patch follows.
Enzo
--------
When verifying a response's signature, the computation will go through
the iov buffer (header + response structs) and the over the page data, to
verify any dynamic data appended to the message (usually on an SMB2 READ
response).
When the response is an error, however, specifically on async reads,
the page data is allocated before receiving the expected data. Being
"valid" data (from the signature computation POV; non-NULL, >0 pages),
it's included in the parsing and generates an invalid signature for the
message.
Fix this by checking if the status is non-zero, and skip the page data
if it is. The issue happens in all protocol versions, and this fix
applies to all.
This issue can be observed with xfstests generic/465.
Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
---
fs/cifs/cifsencrypt.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 46f5718754f9..e3260bb436b3 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -32,15 +32,28 @@ int __cifs_calc_signature(struct smb_rqst *rqst,
int rc;
struct kvec *iov = rqst->rq_iov;
int n_vec = rqst->rq_nvec;
+ bool has_error = false;
/* iov[0] is actual data and not the rfc1002 length for SMB2+ */
if (!is_smb1(server)) {
+ struct smb2_hdr *shdr = (struct smb2_hdr *)iov[0].iov_base;
+
if (iov[0].iov_len <= 4)
return -EIO;
+
+ if (shdr->Status != 0)
+ has_error = true;
+
i = 0;
} else {
+ struct smb_hdr *hdr = (struct smb_hdr *)iov[1].iov_base;
+
if (n_vec < 2 || iov[0].iov_len != 4)
return -EIO;
+
+ if (hdr->Status.CifsError != 0)
+ has_error = true;
+
i = 1; /* skip rfc1002 length */
}
@@ -61,6 +74,9 @@ int __cifs_calc_signature(struct smb_rqst *rqst,
}
}
+ if (has_error)
+ goto out_final;
+
/* now hash over the rq_pages array */
for (i = 0; i < rqst->rq_npages; i++) {
void *kaddr;
@@ -81,6 +97,7 @@ int __cifs_calc_signature(struct smb_rqst *rqst,
kunmap(rqst->rq_pages[i]);
}
+out_final:
rc = crypto_shash_final(shash, signature);
if (rc)
cifs_dbg(VFS, "%s: Could not generate hash\n", __func__);
