| Message ID | 20190716004146.13668-1-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix crash in smb2_compound_op()/smb2_set_next_command() | expand |
----- Original Message ----- > From: "Sasha Levin" <sashal@kernel.org> > To: "Sasha Levin" <sashal@kernel.org>, "Ronnie Sahlberg" <lsahlber@redhat.com>, "linux-cifs" > <linux-cifs@vger.kernel.org> > Cc: "Steve French" <smfrench@gmail.com>, "Stable" <stable@vger.kernel.org>, stable@vger.kernel.org > Sent: Tuesday, 16 July, 2019 11:27:10 AM > Subject: Re: [PATCH] cifs: fix crash in smb2_compound_op()/smb2_set_next_command() > > Hi, > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all > > The bot has tested the following trees: v5.2.1, v5.1.18, v4.19.59, v4.14.133, > v4.9.185, v4.4.185. > > v5.2.1: Build OK! > v5.1.18: Build OK! > v4.19.59: Failed to apply! Possible dependencies: > 271b9c0c8007 ("smb3: Fix rmdir compounding regression to strict servers") > c2e0fe3f5aae ("cifs: make rmdir() use compounding") > c5a5f38f075c ("cifs: add a smb2_compound_op and change QUERY_INFO to use > it") > dcbf91035709 ("cifs: change SMB2_OP_SET_INFO to use compounding") > e77fe73c7e38 ("cifs: we can not use small padding iovs together with > encryption") > f5b05d622a3e ("cifs: add IOCTL for QUERY_INFO passthrough to userspace") > f733e3936da4 ("cifs: change mkdir to use a compound") > f7bfe04bf0db ("cifs: change SMB2_OP_SET_EOF to use compounding") > > v4.14.133: Failed to apply! Possible dependencies: > 2e96467d9eb1 ("cifs: add pdu_size to the TCP_Server_Info structure") > 3d4ef9a15343 ("smb3: fix redundant opens on root") > 730928c8f4be ("cifs: update smb2_queryfs() to use compounding") > 74dcf418fe34 ("CIFS: SMBD: Read correct returned data length for RDMA > write (SMB read) I/O") > 8ce79ec359ad ("cifs: update multiplex loop to handle compounded > responses") > 91cb74f5142c ("cifs: Change SMB2_open to return an iov for the error > parameter") > 93012bf98416 ("cifs: add server->vals->header_preamble_size") > 9d874c36552a ("cifs: fix a buffer leak in smb2_query_symlink") > c5a5f38f075c ("cifs: add a smb2_compound_op and change QUERY_INFO to use > it") > f5b05d622a3e ("cifs: add IOCTL for QUERY_INFO passthrough to userspace") > > v4.9.185: Failed to apply! Possible dependencies: > 31473fc4f965 ("CIFS: Separate SMB2 header structure") > 7fb8986e7449 ("CIFS: Add capability to transform requests before > sending") > 8ce79ec359ad ("cifs: update multiplex loop to handle compounded > responses") > 9bb17e0916a0 ("CIFS: Add transform header handling callbacks") > b8f57ee8aad4 ("CIFS: Separate RFC1001 length processing for SMB2 read") > da502f7df03d ("CIFS: Make SendReceive2() takes resp iov") > ef65aaede23f ("smb2: Enforce sec= mount option") > f5b05d622a3e ("cifs: add IOCTL for QUERY_INFO passthrough to userspace") > > v4.4.185: Failed to apply! Possible dependencies: > 141891f4727c ("SMB3: Add mount parameter to allow user to override max > credits") > 166cea4dc3a4 ("SMB2: Separate RawNTLMSSP authentication from > SMB2_sess_setup") > 16c568efff82 ("cifs: merge the hash calculation helpers") > 275516cdcfa4 ("Print IP address of unresponsive server") > 31473fc4f965 ("CIFS: Separate SMB2 header structure") > 373512ec5c10 ("Prepare for encryption support (first part). Add > decryption and encryption key generation. Thanks to Metze for helping > with this.") > 3baf1a7b9215 ("SMB2: Separate Kerberos authentication from > SMB2_sess_setup") > 7fb8986e7449 ("CIFS: Add capability to transform requests before > sending") > 834170c85978 ("Enable previous version support") > 8ce79ec359ad ("cifs: update multiplex loop to handle compounded > responses") > 9bb17e0916a0 ("CIFS: Add transform header handling callbacks") > adfeb3e00e8e ("cifs: Make echo interval tunable") > da502f7df03d ("CIFS: Make SendReceive2() takes resp iov") > ef65aaede23f ("smb2: Enforce sec= mount option") > f5b05d622a3e ("cifs: add IOCTL for QUERY_INFO passthrough to userspace") > > > NOTE: The patch will not be queued to stable trees until it is upstream. > > How should we proceed with this patch? So it applies cleanly in v5.2.1 and v5.1.18. I think it is sufficient to get into those two versions then. It is very hard to trigger this issue. > -- > Thanks, > Sasha >
diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index 278405d26c47..d8d9cdfa30b6 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -120,6 +120,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, SMB2_O_INFO_FILE, 0, sizeof(struct smb2_file_all_info) + PATH_MAX * 2, 0, NULL); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_query_info_compound_enter(xid, ses->Suid, tcon->tid, @@ -147,6 +149,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, COMPOUND_FID, current->tgid, FILE_DISPOSITION_INFORMATION, SMB2_O_INFO_FILE, 0, data, size); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_rmdir_enter(xid, ses->Suid, tcon->tid, full_path); @@ -163,6 +167,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, COMPOUND_FID, current->tgid, FILE_END_OF_FILE_INFORMATION, SMB2_O_INFO_FILE, 0, data, size); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_set_eof_enter(xid, ses->Suid, tcon->tid, full_path); @@ -180,6 +186,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, COMPOUND_FID, current->tgid, FILE_BASIC_INFORMATION, SMB2_O_INFO_FILE, 0, data, size); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_set_info_compound_enter(xid, ses->Suid, tcon->tid, @@ -206,6 +214,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, COMPOUND_FID, current->tgid, FILE_RENAME_INFORMATION, SMB2_O_INFO_FILE, 0, data, size); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_rename_enter(xid, ses->Suid, tcon->tid, full_path); @@ -231,6 +241,8 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, COMPOUND_FID, current->tgid, FILE_LINK_INFORMATION, SMB2_O_INFO_FILE, 0, data, size); + if (rc) + goto finished; smb2_set_next_command(tcon, &rqst[num_rqst]); smb2_set_related(&rqst[num_rqst++]); trace_smb3_hardlink_enter(xid, ses->Suid, tcon->tid, full_path); diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 4b0b14946343..462890f4cb9c 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2027,6 +2027,10 @@ smb2_set_related(struct smb_rqst *rqst) struct smb2_sync_hdr *shdr; shdr = (struct smb2_sync_hdr *)(rqst->rq_iov[0].iov_base); + if (shdr == NULL) { + cifs_dbg(FYI, "shdr NULL in smb2_set_related\n"); + return; + } shdr->Flags |= SMB2_FLAGS_RELATED_OPERATIONS; } @@ -2041,6 +2045,12 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) unsigned long len = smb_rqst_len(server, rqst); int i, num_padding; + shdr = (struct smb2_sync_hdr *)(rqst->rq_iov[0].iov_base); + if (shdr == NULL) { + cifs_dbg(FYI, "shdr NULL in smb2_set_next_command\n"); + return; + } + /* SMB headers in a compound are 8 byte aligned. */ /* No padding needed */ @@ -2080,7 +2090,6 @@ smb2_set_next_command(struct cifs_tcon *tcon, struct smb_rqst *rqst) } finished: - shdr = (struct smb2_sync_hdr *)(rqst->rq_iov[0].iov_base); shdr->NextCommand = cpu_to_le32(len); }
RHBZ: 1722704 In low memory situations the various SMB2_*_init() functions can fail to allocate a request PDU and thus leave the request iovector as NULL. If we don't check the return code for failure we end up calling smb2_set_next_command() with a NULL iovector causing a crash when it tries to dereference it. CC: Stable <stable@vger.kernel.org> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2inode.c | 12 ++++++++++++ fs/cifs/smb2ops.c | 11 ++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-)