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>] [day] [month] [year] [list]

Message-ID: <3bbd72dff85f3b3b081b1275d50c07dc4fbf62c8.camel@sipsolutions.net>
Date:   Thu, 20 Apr 2023 09:04:26 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Kuniyuki Iwashima <kuniyu@...zon.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v1 net] netlink: Use copy_to_user() for optval in
 netlink_getsockopt().

On Wed, 2023-04-19 at 16:09 -0700, Jakub Kicinski wrote:
> On Wed, 19 Apr 2023 09:17:37 +0200 Johannes Berg wrote:
> > > @@ -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 >= ... :-)
> 
> Can't think of a strong reason either way, so I'd keep the check 
> and len setting as is.

Yeah, fair. The only reason really would be to make it very clear that
using something bigger than an int isn't going to work right.

johannes