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]

Date:   Fri, 28 May 2021 15:39:11 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Xin Long <lucien.xin@...il.com>
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 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.

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