Message-ID: <20230419175258.37172-1-kuniyu@amazon.com>
Date:   Wed, 19 Apr 2023 10:52:58 -0700
From:   Kuniyuki Iwashima <kuniyu@...zon.com>
To:     <johannes@...solutions.net>
CC:     <bspencer@...ckberry.com>, <christophe-h.ricard@...com>,
        <davem@...emloft.net>, <dsahern@...il.com>, <edumazet@...gle.com>,
        <kaber@...sh.net>, <kuba@...nel.org>, <kuni1840@...il.com>,
        <kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
        <pablo@...filter.org>
Subject: Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink_getsockopt().

From:   Johannes Berg <johannes@...solutions.net>
Date:   Wed, 19 Apr 2023 09:17:37 +0200
> On Wed, 2023-04-19 at 00:42 +0000, Kuniyuki Iwashima wrote:
> > Brad Spencer provided a detailed report that when calling getsockopt()
> > for AF_NETLINK, some SOL_NETLINK options set only 1 byte even though such
> > options require more than int as length.
> > 
> > The options return a flag value that fits into 1 byte, but such behaviour
> > confuses users who do not strictly check the value as char.
> 
> Yeah that's iffy. I guess nobody really leaves it uninitialized.
> 
> > Currently, netlink_getsockopt() uses put_user() to copy data to optlen and
> > optval, but put_user() casts the data based on the pointer, char *optval.
> > So, only 1 byte is set to optval.
> 
> Which also means it only ever sets to "1" on little endian systems, on
> big endian it would set to "0x0100'0000" (if initialized to 0 first),
> right?

Yes.


> 
> > To avoid this behaviour, we need to use copy_to_user() or cast optval for
> > put_user().
> 
> Right.
> 
> > Now getsockopt() accepts char as optval as the flags are only 1 byte.
> 
> Personally, I don't think we should allow his. We document (*) and check
> this as taking an int, and while it would _fit_, I don't really think
> it's a good idea to weaken this and allow different types.
> I don't see value in it either, certainly not now since nobody can be
> using it, and not really in the future either since you're not going to
> pack these things in memory, and having an int vs. char on the stack
> really makes basically no difference.
> And when we start seeing code that actually uses this, it'll just be
> more things to support in the userspace API, be more confusing with
> socket options that aren't just flags, etc.
> 
> IOW, I think you should keep the size checks.
> 
> 
> (*) man 7 netlink:
> "Unless otherwise noted, optval is a pointer to an int."

Oh, I didn't know it's documented.

I tried to follow other SOL_XXX handlers, for example, we can
get SO_REUSEADDR as char.  But I'm fine to keep the size check.


> 
> 
> > @@ -1754,39 +1754,17 @@ static int netlink_getsockopt(struct socket *sock, int level, int optname,
> > 
> >         switch (optname) {
> >         case NETLINK_PKTINFO:
> > -               if (len < sizeof(int))
> > -                       return -EINVAL;
> > -               len = sizeof(int);
> 
> On the other hand, this is actually accepting say a u64 now, and then
> sets only 4 bytes of it, though at least it also sets the size to what
> it wrote out.
> 
> So I guess here we can argue either
>  1) keep writing len to 4 and set 4 bytes of the output
>  2) keep the length as is and set all bytes of the output
> 
> but (2) gets confusing if you say used 6 bytes buffer as input? I mean,
> yeah, I'd really hope nobody does that.
> 
> If Jakub is feeling adventurous maybe we should attempt to see if we
> break anything by accepting only == sizeof(int) rather than >= ... :-)

Yes, this is another thing I concerned.  I thought we would have the
same report if we didn't clear the high 32 bits when a user passed u64.

If we want to avoid it, we have to use u64 as val in netlink_getsockopt().
This is even broken for a strange user who passes u128 though :P

> 
> 
> > +       if (put_user(len, optlen) ||
> 
> You never change len now, so there's no point writing it back? Or do we
> somehow need to make sure this is writable? But what for?
> 
> > +           copy_to_user(optval, &val, len))
> 
> There's some magic in copy_to_user() now, but I don't think it will save
> you here - to me this seems really wrong now because 'len' is controlled
> by the user, and sizeof(val) is only 4 bytes - so wouldn't this overrun
> even in the case I mentioned above where the user used a u64 and 'len'
> is actually 8, not 4?

You are right.  I seem to be confused to try to accept char ~ u64 :/
Yes, at least we have to set upper bound for len based on val's actual
size as we do in sk_getsockopt().


> 
> Also, as Jakub points out, even in the case where len *is* 4, you've now
> changed the behaviour on big endian.
> 
> I think that's probably *fine* since the bug meant you basically had to
> initialise to 0 and then check the entire value though, but maybe that
> warrants some discussion in the commit log.

Agreed.


> 
> So my 2 cents:
>  * I wouldn't remove the checks that the size is at least sizeof(int)
>  * I'd - even if it's not strictly backwards compatible - think about
>    restricting to *exactly* sizeof(int), which would make the issue
>    with the copy_to_user() go away as well (**)
>  * if we don't restrict the input length, then need to be much more
>    careful about the copy_to_user() I think, but then what if someone
>    specifies something really huge as the size?

I'm fine either, but I would prefer the latter using u64 for val and
set limit for len as sizeof(u64).


> 
> 
> (**) I only worry slightly that today somebody could've used an
> uninitialised value as the optlen and gotten away with it, but I hope
> that's not the case, that'd be a strange pattern, and if you ever hit 0
> it fails anyway. I'm not really worried someone explicitly wanted to use
> a bigger buffer.
> 
> 
> johannes

Powered by blists - more mailing lists