netdev - Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in netlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Apr 2023 21:46:02 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Kuniyuki Iwashima <kuniyu@...zon.com>
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,
        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().

On Wed, 2023-04-19 at 10:52 -0700, Kuniyuki Iwashima wrote:

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

Yeah ... hence I'm saying we shouldn't bother.

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

That doesn't actually work on big endian, if you have a u64 value 1
that's 00 00 00 00 00 00 00 01, now if you just copy_to_user() that to
an int value (that should've been used) you only get 00 00 00 00 out ...
I guess with the semantics we could technically set it to ff ... ff, but
then there's probably _someone_ out there who expects only 0 or 1 in an
int, or something? So that means to allow a value larger than int
(smaller isn't allowed now) you'd actually have to write some additional
tricky code that checks what the size is and writes it accordingly ...
or just like now writes only the 4 bytes out and sets optlen, but I
suspects that really only works today anyway because everyone uses an
int as documented (and smaller won't work).

So I still think it's better to at least try to just say it _has_ to be
exactly an int, and write that out - and failing that, keep the
behaviour we have today, but hopefully that won't be needed.

johannes