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

[Stefan Metzmacher <metze@samba.org>]

Hi Paulo/Namjae,

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(-)

-- 
2.43.0