From: Steve French <smfrench@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "Aurélien Aptel" <aaptel@suse.com>, CIFS <linux-cifs@vger.kernel.org>
Subject: Re: [bug report] cifs: multichannel: move channel selection above transport layer
Date: Sun, 31 May 2020 14:40:50 -0500 [thread overview]
Message-ID: <CAH2r5mtVaokAwhS8X2PfB12G97B3g1K0vdfwWy=iKRzKxWKFzw@mail.gmail.com> (raw)
In-Reply-To: <20200502132935.GA41736@mwanda>
[-- Attachment #1: Type: text/plain, Size: 2681 bytes --]
Thanks for pointing this out. I don't think tcon->ses can be null
here but might as well add the check in a consistent way to avoid
confusion and static checker warnings
On Sat, May 2, 2020 at 8:32 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Hello Aurelien Aptel,
>
> The patch feeaec621c09: "cifs: multichannel: move channel selection
> above transport layer" from Apr 24, 2020, leads to the following
> static checker warning:
>
> fs/cifs/smb2pdu.c:149 smb2_hdr_assemble()
> error: we previously assumed 'tcon->ses' could be null (see line 133)
>
> fs/cifs/smb2pdu.c
> 120 shdr->ProcessId = cpu_to_le32((__u16)current->tgid);
> 121
> 122 if (!tcon)
> 123 goto out;
> 124
> 125 /* GLOBAL_CAP_LARGE_MTU will only be set if dialect > SMB2.02 */
> 126 /* See sections 2.2.4 and 3.2.4.1.5 of MS-SMB2 */
> 127 if (server && (server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> 128 shdr->CreditCharge = cpu_to_le16(1);
> 129 /* else CreditCharge MBZ */
> 130
> 131 shdr->TreeId = tcon->tid;
> 132 /* Uid is not converted */
> 133 if (tcon->ses)
> ^^^^^^^^^
> Check for NULL.
>
> 134 shdr->SessionId = tcon->ses->Suid;
> 135
> 136 /*
> 137 * If we would set SMB2_FLAGS_DFS_OPERATIONS on open we also would have
> 138 * to pass the path on the Open SMB prefixed by \\server\share.
> 139 * Not sure when we would need to do the augmented path (if ever) and
> 140 * setting this flag breaks the SMB2 open operation since it is
> 141 * illegal to send an empty path name (without \\server\share prefix)
> 142 * when the DFS flag is set in the SMB open header. We could
> 143 * consider setting the flag on all operations other than open
> 144 * but it is safer to net set it for now.
> 145 */
> 146 /* if (tcon->share_flags & SHI1005_FLAGS_DFS)
> 147 shdr->Flags |= SMB2_FLAGS_DFS_OPERATIONS; */
> 148
> 149 if (server && server->sign && !smb3_encryption_required(tcon))
> ^^^^
> Unchecked dereference inside the function. There used to be a bunch
> of checks for NULL "tcon->ses" but some of them were replaces with a
> check for "server" instead.
>
> 150 shdr->Flags |= SMB2_FLAGS_SIGNED;
> 151 out:
> 152 return;
> 153 }
>
> regards,
> dan carpenter
--
Thanks,
Steve
[-- Attachment #2: 0001-smb3-remove-static-checker-warning.patch --]
[-- Type: text/x-patch, Size: 1256 bytes --]
From c8e3c17fd19c998423480cebe183eed2219685fa Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Sun, 31 May 2020 14:36:56 -0500
Subject: [PATCH] smb3: remove static checker warning
Remove static checker warning pointed out by Dan Carpenter:
The patch feeaec621c09: "cifs: multichannel: move channel selection
above transport layer" from Apr 24, 2020, leads to the following
static checker warning:
fs/cifs/smb2pdu.c:149 smb2_hdr_assemble()
error: we previously assumed 'tcon->ses' could be null (see line 133)
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
CC: Aurelien Aptel <aptel@suse.com>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/smb2pdu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 08728c0dcf8b..203a6bd55079 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -85,7 +85,7 @@ static const int smb2_req_struct_sizes[NUMBER_OF_SMB2_COMMANDS] = {
int smb3_encryption_required(const struct cifs_tcon *tcon)
{
- if (!tcon)
+ if (!tcon || !tcon->ses)
return 0;
if ((tcon->ses->session_flags & SMB2_SESSION_FLAG_ENCRYPT_DATA) ||
(tcon->share_flags & SHI1005_FLAGS_ENCRYPT_DATA))
--
2.25.1
prev parent reply other threads:[~2020-05-31 19:41 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-02 13:29 [bug report] cifs: multichannel: move channel selection above transport layer Dan Carpenter
2020-05-31 19:40 ` Steve French [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAH2r5mtVaokAwhS8X2PfB12G97B3g1K0vdfwWy=iKRzKxWKFzw@mail.gmail.com' \
--to=smfrench@gmail.com \
--cc=aaptel@suse.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-cifs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.