[net,0/3] net: start to replace copy_from_sockptr()

Message

Eric Dumazet April 8, 2024, 8:28 a.m. UTC

We got several syzbot reports about unsafe copy_from_sockptr()
calls. After fixing some of them, it appears that we could
use a new helper to factorize all the checks in one place.

This series targets net tree, we can later start converting
many call sites in net-next.

Eric Dumazet (3):
  net: add copy_safe_from_sockptr() helper
  mISDN: fix MISDN_TIME_STAMP handling
  nfc: llcp: fix nfc_llcp_setsockopt() unsafe copies

 drivers/isdn/mISDN/socket.c | 10 +++++-----
 include/linux/sockptr.h     | 25 +++++++++++++++++++++++++
 net/nfc/llcp_sock.c         | 12 ++++++------
 3 files changed, 36 insertions(+), 11 deletions(-)

Comments

[e-mail] April 10, 2024, 12:30 a.m. UTC | #1

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[e-mail]>:

On Mon,  8 Apr 2024 08:28:42 +0000 you wrote:
> We got several syzbot reports about unsafe copy_from_sockptr()
> calls. After fixing some of them, it appears that we could
> use a new helper to factorize all the checks in one place.
> 
> This series targets net tree, we can later start converting
> many call sites in net-next.
> 
> [...]

Here is the summary with links:
  - [net,1/3] net: add copy_safe_from_sockptr() helper
    https://git.kernel.org/netdev/net/c/6309863b31dd
  - [net,2/3] mISDN: fix MISDN_TIME_STAMP handling
    https://git.kernel.org/netdev/net/c/138b787804f4
  - [net,3/3] nfc: llcp: fix nfc_llcp_setsockopt() unsafe copies
    https://git.kernel.org/netdev/net/c/7a87441c9651

You are awesome, thank you!

David Laight April 12, 2024, 5:23 p.m. UTC | #2

From: Eric Dumazet
> Sent: 08 April 2024 09:29
> 
> We got several syzbot reports about unsafe copy_from_sockptr()
> calls. After fixing some of them, it appears that we could
> use a new helper to factorize all the checks in one place.
> 
> This series targets net tree, we can later start converting
> many call sites in net-next.

I have wondered if 'sockptr' should be changed to include the
length and then passed by reference.
That would work 'readonly' for most code, but there are some strange
options that don't obey the expected rules.
At least one has a 'record count' in the 'normal' buffer and the
rest of the data follows - an accident waiting to happen
(especially if called by bpf).

I must find time to prod the bpf people about changing sockptr_t
to be a struct instead of a union - also safer.

The other obvious change is to pull the usercopy for the length
all the way out of getsockopt() to the syscall wrapper.
I started writing a patch to use the return value for the updated
length - but some of the code is too perverted.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)