[PATCH net] udp: fix the len check in udp_lib_getsockopt

[PATCH net] udp: fix the len check in udp_lib_getsockopt

[Xin Long]

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. 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@redhat.com>
Signed-off-by: Xin Long <lucien.xin@gmail.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;
-- 
2.27.0

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[Xin Long]

On Thu, May 27, 2021 at 3:12 PM Xin Long <lucien.xin@gmail.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. 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@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.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;
> --
> 2.27.0
>

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.

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[Jakub Kicinski]

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@gmail.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@redhat.com>
> > Signed-off-by: Xin Long <lucien.xin@gmail.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?

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[Xin Long]

On Fri, May 28, 2021 at 6:39 PM Jakub Kicinski <kuba@kernel.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@gmail.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@redhat.com>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.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.

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[David Ahern]

On 5/28/21 7:47 PM, Xin Long wrote:
> 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.

It is feasible that some app could use bool or u8 for options that have
0 or 1 values and that code has so far worked. This change would break that.

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[Xin Long]

On Fri, May 28, 2021 at 9:57 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 5/28/21 7:47 PM, Xin Long wrote:
> > 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.
>
> It is feasible that some app could use bool or u8 for options that have
> 0 or 1 values and that code has so far worked. This change would break that.
Got it.
Not sure if it's possible or necessary to also return -EINVAL if optlen == 0

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[David Ahern]

On 5/29/21 10:47 AM, Xin Long wrote:
> On Fri, May 28, 2021 at 9:57 PM David Ahern <dsahern@gmail.com> wrote:
>>
>> On 5/28/21 7:47 PM, Xin Long wrote:
>>> 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.
>>
>> It is feasible that some app could use bool or u8 for options that have
>> 0 or 1 values and that code has so far worked. This change would break that.
> Got it.
> Not sure if it's possible or necessary to also return -EINVAL if optlen == 0
> 

do_tcp_getsockopt for example does not fail on optlen 0; no reason to
make this one fail.

Re: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[Xin Long]

On Sun, May 30, 2021 at 9:31 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 5/29/21 10:47 AM, Xin Long wrote:
> > On Fri, May 28, 2021 at 9:57 PM David Ahern <dsahern@gmail.com> wrote:
> >>
> >> On 5/28/21 7:47 PM, Xin Long wrote:
> >>> 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.
> >>
> >> It is feasible that some app could use bool or u8 for options that have
> >> 0 or 1 values and that code has so far worked. This change would break that.
> > Got it.
> > Not sure if it's possible or necessary to also return -EINVAL if optlen == 0
> >
>
> do_tcp_getsockopt for example does not fail on optlen 0; no reason to
> make this one fail.
I was about to say do_tcp_getsockopt has the same issue. :-)

RE: [PATCH net] udp: fix the len check in udp_lib_getsockopt

[David Laight]

From: David Ahern
> Sent: 29 May 2021 02:58
> 
> On 5/28/21 7:47 PM, Xin Long wrote:
> > 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.
> 
> It is feasible that some app could use bool or u8 for options that have
> 0 or 1 values and that code has so far worked. This change would break that.

Especially since the code is also likely to ignore the return
value since the call isn't excepted to actually fail!

Most (but not all) ABI have 'bool' defined the same as 'u32'.
However there will be code that uses 'char' (especially for
setsockopt) and expects it to work.
(And it probably always has done on LE systems.)

A certain other common OS defines the argument as either BOOL
or DWORD - both of which are 32bit.
But I believe it works fine if 'char' is used.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)