linux-wpan.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: 'Eric Dumazet' <eric.dumazet@gmail.com>,
	Christoph Hellwig <hch@lst.de>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Eric Dumazet <edumazet@google.com>
Cc: "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
	"netfilter-devel@vger.kernel.org"
	<netfilter-devel@vger.kernel.org>,
	"coreteam@netfilter.org" <coreteam@netfilter.org>,
	"linux-sctp@vger.kernel.org" <linux-sctp@vger.kernel.org>,
	"linux-hams@vger.kernel.org" <linux-hams@vger.kernel.org>,
	"linux-bluetooth@vger.kernel.org"
	<linux-bluetooth@vger.kernel.org>,
	"bridge@lists.linux-foundation.org" 
	<bridge@lists.linux-foundation.org>,
	"linux-can@vger.kernel.org" <linux-can@vger.kernel.org>,
	"dccp@vger.kernel.org" <dccp@vger.kernel.org>,
	"linux-decnet-user@lists.sourceforge.net" 
	<linux-decnet-user@lists.sourceforge.net>,
	"linux-wpan@vger.kernel.org" <linux-wpan@vger.kernel.org>,
	"linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
	"mptcp@lists.01.org" <mptcp@lists.01.org>,
	"lvs-devel@vger.kernel.org" <lvs-devel@vger.kernel.org>,
	"rds-devel@oss.oracle.com" <rds-devel@oss.oracle.com>,
	"linux-afs@lists.infradead.org" <linux-afs@lists.infradead.org>,
	"tipc-discussion@lists.sourceforge.net" 
	<tipc-discussion@lists.sourceforge.net>,
	"linux-x25@vger.kernel.org" <linux-x25@vger.kernel.org>,
	Stefan Schmidt <stefan@datenfreihafen.org>
Subject: RE: [PATCH 25/26] net: pass a sockptr_t into ->setsockopt
Date: Sat, 8 Aug 2020 13:54:06 +0000	[thread overview]
Message-ID: <ed3741fdf1774cfbbd59d06ecb6994d8@AcuMS.aculab.com> (raw)
In-Reply-To: <90f626a4-d9e5-91a5-b71d-498e3b125da1@gmail.com>

From: Eric Dumazet
> Sent: 07 August 2020 19:29
> 
> On 8/7/20 2:18 AM, David Laight wrote:
> > From: Eric Dumazet
> >> Sent: 06 August 2020 23:21
> >>
> >> On 7/22/20 11:09 PM, Christoph Hellwig wrote:
> >>> Rework the remaining setsockopt code to pass a sockptr_t instead of a
> >>> plain user pointer.  This removes the last remaining set_fs(KERNEL_DS)
> >>> outside of architecture specific code.
> >>>
> >>> Signed-off-by: Christoph Hellwig <hch@lst.de>
> >>> Acked-by: Stefan Schmidt <stefan@datenfreihafen.org> [ieee802154]
> >>> ---
> >>
> >>
> >> ...
> >>
> >>> diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
> >>> index 594e01ad670aa6..874f01cd7aec42 100644
> >>> --- a/net/ipv6/raw.c
> >>> +++ b/net/ipv6/raw.c
> >>> @@ -972,13 +972,13 @@ static int rawv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> >>>  }
> >>>
> >>
> >> ...
> >>
> >>>  static int do_rawv6_setsockopt(struct sock *sk, int level, int optname,
> >>> -			    char __user *optval, unsigned int optlen)
> >>> +			       sockptr_t optval, unsigned int optlen)
> >>>  {
> >>>  	struct raw6_sock *rp = raw6_sk(sk);
> >>>  	int val;
> >>>
> >>> -	if (get_user(val, (int __user *)optval))
> >>> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
> >>>  		return -EFAULT;
> >>>
> >>
> >> converting get_user(...)   to  copy_from_sockptr(...) really assumed the optlen
> >> has been validated to be >= sizeof(int) earlier.
> >>
> >> Which is not always the case, for example here.
> >>
> >> User application can fool us passing optlen=0, and a user pointer of exactly TASK_SIZE-1
> >
> > Won't the user pointer force copy_from_sockptr() to call
> > copy_from_user() which will then do access_ok() on the entire
> > range and so return -EFAULT.
> >
> > The only problems arise if the kernel code adds an offset to the
> > user address.
> > And the later patch added an offset to the copy functions.
> 
> I dunno, I definitely got the following syzbot crash
> 
> No repro found by syzbot yet, but I suspect a 32bit binary program
> did :
> 
> setsockopt(fd, 0x29, 0x24, 0xffffffffffffffff, 0x0)

A few too many ffs...

> BUG: KASAN: wild-memory-access in memcpy include/linux/string.h:406 [inline]
> BUG: KASAN: wild-memory-access in copy_from_sockptr_offset include/linux/sockptr.h:71 [inline]
> BUG: KASAN: wild-memory-access in copy_from_sockptr include/linux/sockptr.h:77 [inline]
> BUG: KASAN: wild-memory-access in do_rawv6_setsockopt net/ipv6/raw.c:1023 [inline]
> BUG: KASAN: wild-memory-access in rawv6_setsockopt+0x1a1/0x6f0 net/ipv6/raw.c:1084
> Read of size 4 at addr 00000000ffffffff by task syz-executor.0/28251

Yep, the code is nearly, but not quite right.
The problem is almost certainly that access_ok(x, 0) always returns success.

In any case the check for a valid user address ought to be exactly
the same one that later selects between copy_to/from_user() and memcpy().

The latter compares the address against 'TASK_SIZE'.
However that isn't the right value either - I think it reads
the value from 'current' that set_fs() sets.
What this code needs is any address that is above the highest
user address and below (or equal to) to lowest kernel one.

On i386 (and probably most 32bit linux) this is 0xc0000000.
On x86-64 this could be any address in the address 'black hole'.
Picking 1ull<<63 may be best.
Quite what the correct #define is requires further research.

There is actually scope for making init_user_sockptr(kern_address)
save a value that will cause copy_to/from_sockptr() go into
the user-copy path with an address that access_ok() will reject.
Then the -EFAULT will get generated in the 'expected' place
and there is no scope for failing to test it's return value.

	David

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

  reply	other threads:[~2020-08-08 13:54 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-23  6:08 get rid of the address_space override in setsockopt v2 Christoph Hellwig
2020-07-23  6:08 ` [PATCH 01/26] bpfilter: fix up a sparse annotation Christoph Hellwig
2020-07-23 11:14   ` Luc Van Oostenryck
2020-07-23  6:08 ` [PATCH 02/26] net/bpfilter: split __bpfilter_process_sockopt Christoph Hellwig
2020-07-23  6:08 ` [PATCH 03/26] bpfilter: reject kernel addresses Christoph Hellwig
2020-07-23 14:42   ` David Laight
2020-07-23 14:44     ` 'Christoph Hellwig'
2020-07-23 14:56       ` David Laight
2020-07-23  6:08 ` [PATCH 04/26] net: add a new sockptr_t type Christoph Hellwig
2020-07-23 15:40   ` Jan Engelhardt
2020-07-23 16:40   ` Eric Dumazet
2020-07-23 16:44     ` Christoph Hellwig
2020-07-23  6:08 ` [PATCH 05/26] net: switch copy_bpf_fprog_from_user to sockptr_t Christoph Hellwig
2020-07-23  6:08 ` [PATCH 06/26] net: switch sock_setbindtodevice " Christoph Hellwig
2020-07-23  6:08 ` [PATCH 07/26] net: switch sock_set_timeout " Christoph Hellwig
2020-07-23  6:08 ` [PATCH 08/26] " Christoph Hellwig
2020-07-23  8:39   ` [MPTCP] " Matthieu Baerts
2020-07-23  6:08 ` [PATCH 09/26] net/xfrm: switch xfrm_user_policy " Christoph Hellwig
2020-07-23  6:08 ` [PATCH 10/26] netfilter: remove the unused user argument to do_update_counters Christoph Hellwig
2020-07-23  6:08 ` [PATCH 11/26] netfilter: switch xt_copy_counters to sockptr_t Christoph Hellwig
2020-07-23  6:08 ` [PATCH 12/26] netfilter: switch nf_setsockopt " Christoph Hellwig
2020-07-27 15:03   ` Jason A. Donenfeld
2020-07-27 15:06     ` Christoph Hellwig
2020-07-27 16:16       ` Jason A. Donenfeld
2020-07-27 16:23         ` Christoph Hellwig
2020-07-28  8:07           ` David Laight
2020-07-28  8:17             ` Jason A. Donenfeld
2020-07-27 16:16     ` Christoph Hellwig
2020-07-27 16:21       ` Jason A. Donenfeld
2020-07-23  6:08 ` [PATCH 13/26] bpfilter: switch bpfilter_ip_set_sockopt " Christoph Hellwig
2020-07-23 11:16   ` David Laight
2020-07-23 11:44     ` 'Christoph Hellwig'
2020-07-23  6:08 ` [PATCH 14/26] net/ipv4: switch ip_mroute_setsockopt " Christoph Hellwig
2020-07-23  6:08 ` [PATCH 15/26] net/ipv4: merge ip_options_get and ip_options_get_from_user Christoph Hellwig
2020-07-23  6:08 ` [PATCH 16/26] net/ipv4: switch do_ip_setsockopt to sockptr_t Christoph Hellwig
2020-07-23  6:08 ` [PATCH 17/26] net/ipv6: switch ip6_mroute_setsockopt " Christoph Hellwig
2020-07-23  6:09 ` [PATCH 18/26] net/ipv6: split up ipv6_flowlabel_opt Christoph Hellwig
2020-07-23  6:09 ` [PATCH 19/26] net/ipv6: switch ipv6_flowlabel_opt to sockptr_t Christoph Hellwig
2020-07-27 12:15   ` Ido Schimmel
2020-07-27 13:00     ` Christoph Hellwig
2020-07-27 13:33       ` Ido Schimmel
2020-07-27 16:15         ` Christoph Hellwig
2020-07-27 18:22           ` Ido Schimmel
2020-07-27 13:24     ` David Laight
2020-07-23  6:09 ` [PATCH 20/26] net/ipv6: factor out a ipv6_set_opt_hdr helper Christoph Hellwig
2020-07-23  6:09 ` [PATCH 21/26] net/ipv6: switch do_ipv6_setsockopt to sockptr_t Christoph Hellwig
2020-07-23  6:09 ` [PATCH 22/26] net/udp: switch udp_lib_setsockopt " Christoph Hellwig
2020-07-23  6:09 ` [PATCH 23/26] net/tcp: switch ->md5_parse " Christoph Hellwig
2020-07-23  6:09 ` [PATCH 24/26] net/tcp: switch do_tcp_setsockopt " Christoph Hellwig
2020-07-23  6:09 ` [PATCH 25/26] net: pass a sockptr_t into ->setsockopt Christoph Hellwig
2020-07-23  8:39   ` [MPTCP] " Matthieu Baerts
2020-08-06 22:21   ` Eric Dumazet
2020-08-07  7:21     ` Christoph Hellwig
2020-08-07  9:18     ` David Laight
2020-08-07 18:29       ` Eric Dumazet
2020-08-08 13:54         ` David Laight [this message]
2020-07-23  6:09 ` [PATCH 26/26] net: optimize the sockptr_t for unified kernel/user address spaces Christoph Hellwig
2020-07-24 22:43 ` get rid of the address_space override in setsockopt v2 David Miller
2020-07-26  7:03   ` Christoph Hellwig
2020-07-26  7:08     ` Andreas Schwab
2020-07-26  7:46     ` David Miller
2020-07-27  9:51   ` David Laight
2020-07-27 13:48     ` Al Viro
2020-07-27 14:09       ` David Laight

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ed3741fdf1774cfbbd59d06ecb6994d8@AcuMS.aculab.com \
    --to=david.laight@aculab.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=bridge@lists.linux-foundation.org \
    --cc=coreteam@netfilter.org \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dccp@vger.kernel.org \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=hch@lst.de \
    --cc=kuba@kernel.org \
    --cc=kuznet@ms2.inr.ac.ru \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-can@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-decnet-user@lists.sourceforge.net \
    --cc=linux-hams@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=linux-wpan@vger.kernel.org \
    --cc=linux-x25@vger.kernel.org \
    --cc=lvs-devel@vger.kernel.org \
    --cc=mptcp@lists.01.org \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=rds-devel@oss.oracle.com \
    --cc=stefan@datenfreihafen.org \
    --cc=tipc-discussion@lists.sourceforge.net \
    --cc=yoshfuji@linux-ipv6.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).