Re: [PATCH net-next v3 00/16] l2tp: fix API races discovered by syzbot

[David Miller]

From: James Chapman 
Date: Mon, 12 Feb 2018 17:33:23 +

> This patch series addresses several races with L2TP APIs discovered by
> syzbot. While working on this, it became clear that the L2TP code
> needed some work to address object lifetime issues. There are no
> functional changes.
> 
> The set of patches 1-13 in combination fix the following syzbot reports.
> 
> 9df43faf0 KASAN: use-after-free Read in pppol2tp_connect
> 6e6a5ec8d general protection fault in pppol2tp_connect
> 347bd5acd KASAN: use-after-free Read in inet_shutdown
> 19c09769f WARNING in debug_print_object

Some symbol export issues:

ERROR: "l2tp_tunnel_free" [net/l2tp/l2tp_ppp.ko] undefined!
ERROR: "l2tp_tunnel_free" [net/l2tp/l2tp_netlink.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1

Also, this series is also a hodge-podge of bug fixes that really belong
in 'net' alongside cleanups and refactoring that belong in 'net-next'.

Can you please pull out the genuine bug fixes into a smaller series
targetting 'net' and then after you fix the symbol export issues
we can build on top of that in net-next with the cleanups and
refactoring.

Thank you.

[PATCH net-next v3 00/16] l2tp: fix API races discovered by syzbot

[James Chapman]

This patch series addresses several races with L2TP APIs discovered by
syzbot. While working on this, it became clear that the L2TP code
needed some work to address object lifetime issues. There are no
functional changes.

The set of patches 1-13 in combination fix the following syzbot reports.

9df43faf0 KASAN: use-after-free Read in pppol2tp_connect
6e6a5ec8d general protection fault in pppol2tp_connect
347bd5acd KASAN: use-after-free Read in inet_shutdown
19c09769f WARNING in debug_print_object

In detail:-

 1. Add RCU protection of sk_user_data. Since L2TP hooks on sockets
opened by userspace, we may race with other socket families that
attempt to use the same socket. (patches 1-2)

 2. Fix inet_shutdown races when L2TP tunnels close. (patch 3)

 3. Refactor code to address internal object lifetime
issues. Previously internal refcounts and socket refcounts were
used inconsistently and led to workarounds to fix specific
bugs. With the changes made here, we can now fetch the
tunnel/session context from its socket sk_user_data and fetch the
socket from the tunnel/session without using other APIs such as
sockfd_lookup. (patches 4-8)

 4. Refactor pppol2tp_connect to fix several races and split it up to
improve readability. (patch 9)

 5. Refactor session destroy paths to use a workqueue such that all
session cleanup is done using common code, regardless of whether
the session is closed by netlink request or (in the case of ppp)
its socket closed. (patches 10-13)

 6. Misc cleanups made possible by the refactoring done in this
series. (patches 14-16)

Changes in v2:-

Fix compile error that would have broken bisect.

Changes in v3:-

Reformat some commit logs to move any oops text into commit body,
move fixes/reported-by tags to end of commit text and add my
signoff which got dropped in v2.

James Chapman (16):
  l2tp: update sk_user_data while holding sk_callback_lock
  l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy
  l2tp: don't use inet_shutdown on tunnel destroy
  l2tp: refactor tunnel lifetime handling wrt its socket
  l2tp: use tunnel closing flag
  l2tp: refactor session lifetime handling
  l2tp: hide sessions if they are closing
  l2tp: hide session from pppol2tp_sock_to_session if it is closing
  l2tp: refactor pppol2tp_connect
  l2tp: add session_free callback
  l2tp: do session destroy using a workqueue
  l2tp: simplify l2tp_tunnel_closeall
  l2tp: refactor ppp session cleanup paths
  l2tp: remove redundant sk_user_data check when creating tunnels
  l2tp: remove unwanted error message
  l2tp: make __l2tp_session_unhash internal

 net/l2tp/l2tp_core.c | 310 ++--
 net/l2tp/l2tp_core.h |  37 ++---
 net/l2tp/l2tp_ip.c   |  10 +-
 net/l2tp/l2tp_ip6.c  |   8 +-
 net/l2tp/l2tp_ppp.c  | 434 ++-
 5 files changed, 434 insertions(+), 365 deletions(-)

-- 
1.9.1