Re: [PATCH 0/5] smb: client: don't use server->smbd_conn without a reference or lock

[Stefan Metzmacher <metze@samba.org>]

Hi Paulo/Namjae,

I'll address these https://sashiko.dev/#/patchset/cover.1791225453.git.metze%40samba.org
and resubmit v2 later...

metze

> The SMB Direct client keeps the transport in server->smbd_conn and
> accesses it from several places without any locking:
> 
>    - smbd_get_parameters()
>    - smbd_register_mr()
>    - smbd_debug_proc_show()
> 
> At the same time cifs_abort_connection() (and the reconnect path) can
> call smbd_destroy() at any time, which frees the smbd_connection and
> releases the underlying smbdirect socket. So the readers above can race
> with the teardown and end up using a freed connection/socket, a
> use-after-free.
> 
> While looking at this, smbd_get_parameters() also turned out to
> dereference server->smbd_conn without checking it, which is a NULL
> pointer dereference in smb2_negotiate_wsize()/smb2_negotiate_rsize()
> when it is called during a reconnect window.
> 
> This series closes the races:
> 
>    - server->smbd_conn is only ever changed under server->srv_lock, so
>      readers can safely look at it while holding that lock.
> 
>    - smbdirect gets smbdirect_socket_get()/smbdirect_socket_put(), which
>      take and drop a reference that keeps the socket memory alive (but
>      does not keep it connected). smbd_register_mr() uses them to hold
>      the socket across smbdirect_connection_register_mr_io(); once the
>      connection is gone the socket is no longer connected and the call
>      fails gracefully. A registered MR has its own reference counting.
> 
>    - smbd_get_parameters() no longer returns a pointer into the socket
>      (which cannot be kept valid for the callers). It copies the current
>      parameters into the new server->smbd_params under server->srv_lock
>      and returns a pointer to that; with no connection it returns zeros.
> 
>    - smbd_debug_proc_show() runs with cifs_tcp_ses_lock held, so it takes
>      server->srv_lock instead of a socket reference.
> 
> The two "pass server to ..." patches are mechanical prototype changes
> with no behaviour change, split out so the final fix stays small and
> each step builds on its own.
> 
> All of this is a use-after-free that goes back to the switch to the
> smbdirect socket API, so the whole series is tagged for stable via
> 
>    Fixes: b8aef8c8808c ("smb: client: make use of smbdirect_socket_create_kern()/smbdirect_socket_release()")
> 
> which is in mainline since v7.1-rc1.
> 
> I run various xfstests with this.
> 
> I'm unsure if this should go via cifs-next or ksmbd-for-next,
> for consistency with the series I just send I guess both
> could go via ksmbd-for-next?
> 
> Stefan Metzmacher (5):
>    smb: client: change server->smbd_conn only under server->srv_lock
>    smb: smbdirect: add smbdirect_socket_get() and smbdirect_socket_put()
>    smb: client: pass server to smbd_get_parameters()
>    smb: client: pass server to smbd_register_mr()
>    smb: client: don't use server->smbd_conn without a reference or lock
> 
>   fs/smb/client/cifsglob.h  |  12 ++++
>   fs/smb/client/connect.c   |  12 +++-
>   fs/smb/client/file.c      |   4 +-
>   fs/smb/client/smb2ops.c   |   4 +-
>   fs/smb/client/smb2pdu.c   |   4 +-
>   fs/smb/client/smbdirect.c | 124 +++++++++++++++++++++++++++++++++-----
>   fs/smb/client/smbdirect.h |   8 ++-
>   fs/smb/smbdirect/socket.c |  27 +++++++++
>   include/linux/smbdirect.h |   3 +
>   9 files changed, 171 insertions(+), 27 deletions(-)
>