netdev - Re: [PATCH net] udp: fix the len check in udp_lib

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]

Message-ID: <CADvbK_dvj2ywH5nQGcsjAWOKb5hdLfoVnjKNmLsstk3R1j7MyA@mail.gmail.com>
Date:   Fri, 28 May 2021 21:47:06 -0400
From:   Xin Long <lucien.xin@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     network dev <netdev@...r.kernel.org>, davem <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        David Ahern <dsahern@...il.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

On Fri, May 28, 2021 at 6:39 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Thu, 27 May 2021 15:13:32 -0400 Xin Long wrote:
> > On Thu, May 27, 2021 at 3:12 PM Xin Long <lucien.xin@...il.com> wrote:
> > > Currently, when calling UDP's getsockopt, it only makes sure 'len'
> > > is not less than 0, then copys 'len' bytes back to usespace while
> > > all data is 'int' type there.
> > >
> > > If userspace sets 'len' with a value N (N < sizeof(int)), it will
> > > only copy N bytes of the data to userspace with no error returned,
> > > which doesn't seem right.
>
> I'm not so sure of that. In cases where the value returned may grow
> with newer kernel releases truncating the output to the size of buffer
> user space provided is pretty normal. I think this code is working as
> intended.
Hard to say, I saw this kind of checks from 1da177e4c3f4 ("Linux-2.6.12-rc2"),
the new codes are using 'len < sizeof(x)'. Comparing to growing 'int', other
structures are more likely to grow.

>
> > > Like in Chen Yi's case where N is 0, it
> > > called getsockopt and got an incorrect value but with no error
> > > returned.
> > >
> > > The patch is to fix the len check and make sure it's not less than
> > > sizeof(int). Otherwise, '-EINVAL' is returned, as it does in other
> > > protocols like SCTP/TIPC.
> > >
> > > Reported-by: Chen Yi <yiche@...hat.com>
> > > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > > ---
> > >  net/ipv4/udp.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > > index 15f5504adf5b..90de2ac70ea9 100644
> > > --- a/net/ipv4/udp.c
> > > +++ b/net/ipv4/udp.c
> > > @@ -2762,11 +2762,11 @@ int udp_lib_getsockopt(struct sock *sk, int level, int optname,
> > >         if (get_user(len, optlen))
> > >                 return -EFAULT;
> > >
> > > -       len = min_t(unsigned int, len, sizeof(int));
> > > -
> > > -       if (len < 0)
> > > +       if (len < sizeof(int))
> > >                 return -EINVAL;
> > >
> > > +       len = sizeof(int);
> > > +
> > >         switch (optname) {
> > >         case UDP_CORK:
> > >                 val = up->corkflag;
> >
> > Note I'm not sure if this fix may break any APP, but the current
> > behavior definitely is not correct and doesn't match the man doc
> > of getsockopt, so please review.
>
> Can you quote the part of man getsockopt you're referring to?
The partial byte(or even 0) of the value returned due to passing a wrong
optlen should be considered as an error. "On error, -1 is returned, and
errno is set appropriately.". Success returned in that case only confuses
the user.