Message-ID: <d098026456c8393463e6cf33195edc19369c220b.camel@sipsolutions.net>
Date:   Wed, 19 Apr 2023 09:17:37 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     Patrick McHardy <kaber@...sh.net>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Christophe Ricard <christophe-h.ricard@...com>,
        David Ahern <dsahern@...il.com>,
        Kuniyuki Iwashima <kuni1840@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Brad Spencer <bspencer@...ckberry.com>
Subject: Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in
 netlink_getsockopt().

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?

> 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."


> @@ -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 >= ... :-)


> +       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? 

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.

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