| Message ID | 20251226201939.36293-1-aadityakansal390@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | smb: client: terminate session upon signature verification failure | expand |
On 12/27, Aaditya Kansal wrote: >Currently, when the SMB signature verification fails, the error is >logged without any action to terminate the session. > >Call cifs_reconnect() to terminate the session if the signature >verification fails. > >Signed-off-by: Aaditya Kansal <aadityakansal390@gmail.com> Thanks, I think this was long overdue. >--- > fs/smb/client/cifstransport.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c >index 28d1cee90625..89818bb983ec 100644 >--- a/fs/smb/client/cifstransport.c >+++ b/fs/smb/client/cifstransport.c >@@ -169,12 +169,15 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server, > > iov[0].iov_base = mid->resp_buf; > iov[0].iov_len = len; >- /* FIXME: add code to kill session */ >+ > rc = cifs_verify_signature(&rqst, server, > mid->sequence_number); >- if (rc) >+ if (rc) { > cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n", > rc); >+ cifs_reconnect(server, true); I'd like to hear opinions on having reconnect happen only if signing is required by server, otherwise only log the error (current behaviour). >+ return rc; >+ } > } > > /* BB special case reconnect tid and uid here? */ >-- Nonetheless, Acked-by: Enzo Matsumiya <ematsumiya@suse.de> Cheers,
On Mon, Jan 5, 2026 at 10:37 AM Enzo Matsumiya <ematsumiya@suse.de> wrote: > > On 12/27, Aaditya Kansal wrote: > >Currently, when the SMB signature verification fails, the error is > >logged without any action to terminate the session. > > > >Call cifs_reconnect() to terminate the session if the signature > >verification fails. > > > >Signed-off-by: Aaditya Kansal <aadityakansal390@gmail.com> > > Thanks, I think this was long overdue. > > >--- > > fs/smb/client/cifstransport.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > >diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c > >index 28d1cee90625..89818bb983ec 100644 > >--- a/fs/smb/client/cifstransport.c > >+++ b/fs/smb/client/cifstransport.c > >@@ -169,12 +169,15 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server, > > > > iov[0].iov_base = mid->resp_buf; > > iov[0].iov_len = len; > >- /* FIXME: add code to kill session */ > >+ > > rc = cifs_verify_signature(&rqst, server, > > mid->sequence_number); > >- if (rc) > >+ if (rc) { > > cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n", > > rc); > >+ cifs_reconnect(server, true); > > I'd like to hear opinions on having reconnect happen only if signing > is required by server, otherwise only log the error (current behaviour). I was thinking the reverse - if the signature verification on the client fails but it was the server, not the client, who forced signing, then we could skip the reconnect (and just log), but if client forces signing (sign mount option eg. or /proc/fs/cifs/SecuritfyFlags setting forcing signing) then we should be more strict and reconnect > >+ return rc; > >+ } > > } > > > > /* BB special case reconnect tid and uid here? */ > >-- > > Nonetheless, > > Acked-by: Enzo Matsumiya <ematsumiya@suse.de> > > > Cheers, >
On 1/5/26 22:30, Steve French wrote: > On Mon, Jan 5, 2026 at 10:37 AM Enzo Matsumiya <ematsumiya@suse.de> wrote: >> On 12/27, Aaditya Kansal wrote: >>> Currently, when the SMB signature verification fails, the error is >>> logged without any action to terminate the session. >>> >>> Call cifs_reconnect() to terminate the session if the signature >>> verification fails. >>> >>> Signed-off-by: Aaditya Kansal <aadityakansal390@gmail.com> >> Thanks, I think this was long overdue. >> Thank you for acknowledgement. >>> --- >>> fs/smb/client/cifstransport.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c >>> index 28d1cee90625..89818bb983ec 100644 >>> --- a/fs/smb/client/cifstransport.c >>> +++ b/fs/smb/client/cifstransport.c >>> @@ -169,12 +169,15 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server, >>> >>> iov[0].iov_base = mid->resp_buf; >>> iov[0].iov_len = len; >>> - /* FIXME: add code to kill session */ >>> + >>> rc = cifs_verify_signature(&rqst, server, >>> mid->sequence_number); >>> - if (rc) >>> + if (rc) { >>> cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n", >>> rc); >>> + cifs_reconnect(server, true); >> I'd like to hear opinions on having reconnect happen only if signing >> is required by server, otherwise only log the error (current behaviour). > I was thinking the reverse - if the signature verification on the > client fails but it was the server, > not the client, who forced signing, then we could skip the reconnect > (and just log), but if > client forces signing (sign mount option eg. or > /proc/fs/cifs/SecuritfyFlags setting forcing > signing) then we should be more strict and reconnect > Thank you both for replying amidst holidays. I understand what you are saying. I am happy to revise the patch. Which version should I go with: reconnecting when server forces signing or the reverse? >>> + return rc; >>> + } >>> } >>> >>> /* BB special case reconnect tid and uid here? */ >>> -- >> Nonetheless, >> >> Acked-by: Enzo Matsumiya <ematsumiya@suse.de> >> >> >> Cheers, >> >
If the client required signing (on mount or via SecurityFlags), I think it is reasonable to terminate the session and reconnect on signature verification failure. If the server (but not the client) required signing but screwed up the signature it generates (due to server bug presumably) seems ok to keep current behavior (presumably we ignore the bad signature and just log it) On Tue, Jan 6, 2026 at 10:03 AM Aaditya Kansal <aadityakansal390@gmail.com> wrote: > > > > On 1/5/26 22:30, Steve French wrote: > > On Mon, Jan 5, 2026 at 10:37 AM Enzo Matsumiya <ematsumiya@suse.de> wrote: > >> On 12/27, Aaditya Kansal wrote: > >>> Currently, when the SMB signature verification fails, the error is > >>> logged without any action to terminate the session. > >>> > >>> Call cifs_reconnect() to terminate the session if the signature > >>> verification fails. > >>> > >>> Signed-off-by: Aaditya Kansal <aadityakansal390@gmail.com> > >> Thanks, I think this was long overdue. > >> > Thank you for acknowledgement. > >>> --- > >>> fs/smb/client/cifstransport.c | 7 +++++-- > >>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c > >>> index 28d1cee90625..89818bb983ec 100644 > >>> --- a/fs/smb/client/cifstransport.c > >>> +++ b/fs/smb/client/cifstransport.c > >>> @@ -169,12 +169,15 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server, > >>> > >>> iov[0].iov_base = mid->resp_buf; > >>> iov[0].iov_len = len; > >>> - /* FIXME: add code to kill session */ > >>> + > >>> rc = cifs_verify_signature(&rqst, server, > >>> mid->sequence_number); > >>> - if (rc) > >>> + if (rc) { > >>> cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n", > >>> rc); > >>> + cifs_reconnect(server, true); > >> I'd like to hear opinions on having reconnect happen only if signing > >> is required by server, otherwise only log the error (current behaviour). > > I was thinking the reverse - if the signature verification on the > > client fails but it was the server, > > not the client, who forced signing, then we could skip the reconnect > > (and just log), but if > > client forces signing (sign mount option eg. or > > /proc/fs/cifs/SecuritfyFlags setting forcing > > signing) then we should be more strict and reconnect > > > Thank you both for replying amidst holidays. I understand what you are > saying. I am > happy to revise the patch. Which version should I go with: reconnecting > when server > forces signing or the reverse? > >>> + return rc; > >>> + } > >>> } > >>> > >>> /* BB special case reconnect tid and uid here? */ > >>> -- > >> Nonetheless, > >> > >> Acked-by: Enzo Matsumiya <ematsumiya@suse.de> > >> > >> > >> Cheers, > >> > > > -- > -AK > >
diff --git a/fs/smb/client/cifstransport.c b/fs/smb/client/cifstransport.c index 28d1cee90625..89818bb983ec 100644 --- a/fs/smb/client/cifstransport.c +++ b/fs/smb/client/cifstransport.c @@ -169,12 +169,15 @@ cifs_check_receive(struct mid_q_entry *mid, struct TCP_Server_Info *server, iov[0].iov_base = mid->resp_buf; iov[0].iov_len = len; - /* FIXME: add code to kill session */ + rc = cifs_verify_signature(&rqst, server, mid->sequence_number); - if (rc) + if (rc) { cifs_server_dbg(VFS, "SMB signature verification returned error = %d\n", rc); + cifs_reconnect(server, true); + return rc; + } } /* BB special case reconnect tid and uid here? */
Currently, when the SMB signature verification fails, the error is logged without any action to terminate the session. Call cifs_reconnect() to terminate the session if the signature verification fails. Signed-off-by: Aaditya Kansal <aadityakansal390@gmail.com> --- fs/smb/client/cifstransport.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)