[PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Ani Sinha]

Hi guys :

I am looking at the thread :

 [PATCH] net: socket: error on a negative msg_namelen

and the patch that was submitted in that thread :

commit dbb490b96584d4e958533fb637f08b557f505657
Author: Matthew Leach <matthew.leach@arm.com>
Date:   Tue Mar 11 11:58:27 2014 +0000

    net: socket: error on a negative msg_namelen


According to the linux recvmsg manpage, the caller of recvmsg() may
set msg_name to NULL if he does not care about source address but the
manpage does not say that one has to set msg_namelen to 0 in this
case. Essentially msg_namelen is a don't care if msg_name is NULL. I
think in the kernel, we should validate msg_namelen only if the caller
has also set msg_name and return EINVAL only when msg_name is non-null
and msg_namelen is negative.

The following patch will do the intended :


>From ef8e8bd78635ac677f2d4b76fec9990ed1db763c Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@aristanetworks.com>
Date: Fri, 5 Sep 2014 13:25:22 -0700
Subject:[PATCH] net: socket: do not validate msg_namelen unless
msg_name is non-NULL

The value of msg_namelen in msghdr structure is irrelevant
 when msg_name is NULL. We should not validate the value
 passed in msg_namelen unless msg_name is non-NULL.

Signed-off-by: Ani Sinha <ani@aristanetworks.com>
---
 net/socket.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 95ee7d8..a5dfe01 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1997,7 +1997,7 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
  if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
  return -EFAULT;

- if (kmsg->msg_namelen < 0)
+ if (kmsg->msg_name && kmsg->msg_namelen < 0)
  return -EINVAL;

  if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
-- 
1.7.4.4

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Hannes Frederic Sowa]

Hi,

On Fri, Sep 5, 2014, at 23:00, Ani Sinha wrote:
> Hi guys :
> 
> I am looking at the thread :
> 
>  [PATCH] net: socket: error on a negative msg_namelen
> 
> and the patch that was submitted in that thread :
> 
> commit dbb490b96584d4e958533fb637f08b557f505657
> Author: Matthew Leach <matthew.leach@arm.com>
> Date:   Tue Mar 11 11:58:27 2014 +0000
> 
>     net: socket: error on a negative msg_namelen
> 
> 
> According to the linux recvmsg manpage, the caller of recvmsg() may
> set msg_name to NULL if he does not care about source address but the
> manpage does not say that one has to set msg_namelen to 0 in this
> case. Essentially msg_namelen is a don't care if msg_name is NULL. I
> think in the kernel, we should validate msg_namelen only if the caller
> has also set msg_name and return EINVAL only when msg_name is non-null
> and msg_namelen is negative.
> 
> The following patch will do the intended :
> 
> 
> From ef8e8bd78635ac677f2d4b76fec9990ed1db763c Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@aristanetworks.com>
> Date: Fri, 5 Sep 2014 13:25:22 -0700
> Subject:[PATCH] net: socket: do not validate msg_namelen unless
> msg_name is non-NULL
> 
> The value of msg_namelen in msghdr structure is irrelevant
>  when msg_name is NULL. We should not validate the value
>  passed in msg_namelen unless msg_name is non-NULL.
> 
> Signed-off-by: Ani Sinha <ani@aristanetworks.com>
> ---
>  net/socket.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8..a5dfe01 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1997,7 +1997,7 @@ static int copy_msghdr_from_user(struct msghdr
> *kmsg,
>   if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
>   return -EFAULT;
> 
> - if (kmsg->msg_namelen < 0)
> + if (kmsg->msg_name && kmsg->msg_namelen < 0)
>   return -EINVAL;
> 
>   if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))

The reason for the above mentioned commit was the signed/unsigned
conversion by this check. To not trigger any static checker tools, I
would suggest to just set kmsg->msg_namelen to zero in case msg_name is
NULL.

Thanks,
Hannes

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Ani Sinha]

On Fri, Sep 5, 2014 at 2:14 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> Hi,
>
> On Fri, Sep 5, 2014, at 23:00, Ani Sinha wrote:
>> Hi guys :
>>
>> I am looking at the thread :
>>
>>  [PATCH] net: socket: error on a negative msg_namelen
>>
>> and the patch that was submitted in that thread :
>>
>> commit dbb490b96584d4e958533fb637f08b557f505657
>> Author: Matthew Leach <matthew.leach@arm.com>
>> Date:   Tue Mar 11 11:58:27 2014 +0000
>>
>>     net: socket: error on a negative msg_namelen
>>
>>
>> According to the linux recvmsg manpage, the caller of recvmsg() may
>> set msg_name to NULL if he does not care about source address but the
>> manpage does not say that one has to set msg_namelen to 0 in this
>> case. Essentially msg_namelen is a don't care if msg_name is NULL. I
>> think in the kernel, we should validate msg_namelen only if the caller
>> has also set msg_name and return EINVAL only when msg_name is non-null
>> and msg_namelen is negative.
>>
>> The following patch will do the intended :
>>
>>
>> From ef8e8bd78635ac677f2d4b76fec9990ed1db763c Mon Sep 17 00:00:00 2001
>> From: Ani Sinha <ani@aristanetworks.com>
>> Date: Fri, 5 Sep 2014 13:25:22 -0700
>> Subject:[PATCH] net: socket: do not validate msg_namelen unless
>> msg_name is non-NULL
>>
>> The value of msg_namelen in msghdr structure is irrelevant
>>  when msg_name is NULL. We should not validate the value
>>  passed in msg_namelen unless msg_name is non-NULL.
>>
>> Signed-off-by: Ani Sinha <ani@aristanetworks.com>
>> ---
>>  net/socket.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index 95ee7d8..a5dfe01 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -1997,7 +1997,7 @@ static int copy_msghdr_from_user(struct msghdr
>> *kmsg,
>>   if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
>>   return -EFAULT;
>>
>> - if (kmsg->msg_namelen < 0)
>> + if (kmsg->msg_name && kmsg->msg_namelen < 0)
>>   return -EINVAL;
>>
>>   if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
>
> The reason for the above mentioned commit was the signed/unsigned
> conversion by this check. To not trigger any static checker tools, I
> would suggest to just set kmsg->msg_namelen to zero in case msg_name is
> NULL.

I suspect any code that was previously written without taking into
account this new restriction will now begin to fail. For some of them,
we may not have the freedom to change the code as per this new
restrictions. Since the manpage did not enforce this, the developers
can not be blamed for not setting namelen when passing name with NULL
value.

Cheers,
ani

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Hannes Frederic Sowa]

On Fr, 2014-09-05 at 14:21 -0700, Ani Sinha wrote:
> On Fri, Sep 5, 2014 at 2:14 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi,
> >
> > On Fri, Sep 5, 2014, at 23:00, Ani Sinha wrote:
> >> Hi guys :
> >>
> >> I am looking at the thread :
> >>
> >>  [PATCH] net: socket: error on a negative msg_namelen
> >>
> >> and the patch that was submitted in that thread :
> >>
> >> commit dbb490b96584d4e958533fb637f08b557f505657
> >> Author: Matthew Leach <matthew.leach@arm.com>
> >> Date:   Tue Mar 11 11:58:27 2014 +0000
> >>
> >>     net: socket: error on a negative msg_namelen
> >>
> >>
> >> According to the linux recvmsg manpage, the caller of recvmsg() may
> >> set msg_name to NULL if he does not care about source address but the
> >> manpage does not say that one has to set msg_namelen to 0 in this
> >> case. Essentially msg_namelen is a don't care if msg_name is NULL. I
> >> think in the kernel, we should validate msg_namelen only if the caller
> >> has also set msg_name and return EINVAL only when msg_name is non-null
> >> and msg_namelen is negative.
> >>
> >> The following patch will do the intended :
> >>
> >>
> >> From ef8e8bd78635ac677f2d4b76fec9990ed1db763c Mon Sep 17 00:00:00 2001
> >> From: Ani Sinha <ani@aristanetworks.com>
> >> Date: Fri, 5 Sep 2014 13:25:22 -0700
> >> Subject:[PATCH] net: socket: do not validate msg_namelen unless
> >> msg_name is non-NULL
> >>
> >> The value of msg_namelen in msghdr structure is irrelevant
> >>  when msg_name is NULL. We should not validate the value
> >>  passed in msg_namelen unless msg_name is non-NULL.
> >>
> >> Signed-off-by: Ani Sinha <ani@aristanetworks.com>
> >> ---
> >>  net/socket.c |    2 +-
> >>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/net/socket.c b/net/socket.c
> >> index 95ee7d8..a5dfe01 100644
> >> --- a/net/socket.c
> >> +++ b/net/socket.c
> >> @@ -1997,7 +1997,7 @@ static int copy_msghdr_from_user(struct msghdr
> >> *kmsg,
> >>   if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> >>   return -EFAULT;
> >>
> >> - if (kmsg->msg_namelen < 0)
> >> + if (kmsg->msg_name && kmsg->msg_namelen < 0)
> >>   return -EINVAL;
> >>
> >>   if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> >
> > The reason for the above mentioned commit was the signed/unsigned
> > conversion by this check. To not trigger any static checker tools, I
> > would suggest to just set kmsg->msg_namelen to zero in case msg_name is
> > NULL.
> 
> I suspect any code that was previously written without taking into
> account this new restriction will now begin to fail. For some of them,
> we may not have the freedom to change the code as per this new
> restrictions. Since the manpage did not enforce this, the developers
> can not be blamed for not setting namelen when passing name with NULL
> value.

Yes, I understood. Same issues with sin6_flowinfo where a specific
setsockopt is needed so the kernel will look at it at all.

If you set msg_namelen = 0 if msg_name == NULL prior to the < 0 check it
should not trigger the return -EINVAL and also we don't run into the
unsafe implicit conversion case when comparing msg_namelen with the
result of the sizeof(). Do you see any problems with that?

Thanks,
Hannes

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Eric Dumazet]

On Fri, 2014-09-05 at 23:26 +0200, Hannes Frederic Sowa wrote:

> Yes, I understood. Same issues with sin6_flowinfo where a specific
> setsockopt is needed so the kernel will look at it at all.
> 
> If you set msg_namelen = 0 if msg_name == NULL prior to the < 0 check it
> should not trigger the return -EINVAL and also we don't run into the
> unsafe implicit conversion case when comparing msg_namelen with the
> result of the sizeof(). Do you see any problems with that?

BTW, it looks some of the tests are done in different places, like
verify_iovec()

We certainly can cleanup the confusion.

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Ani Sinha]

On Fri, Sep 5, 2014 at 2:26 PM, Hannes Frederic Sowa
> If you set msg_namelen = 0 if msg_name == NULL prior to the < 0 check it
> should not trigger the return -EINVAL and also we don't run into the
> unsafe implicit conversion case when comparing msg_namelen with the
> result of the sizeof(). Do you see any problems with that?

yes, sorry I misunderstood you. Here's the updated patch :

>From ea39174d4475d7def61410210613ab24a4ce0e81 Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@aristanetworks.com>
Date: Fri, 5 Sep 2014 14:33:20 -0700
Subject: [PATCH] net:socket: set msg_namelen to 0 if msg_name is
passed as NULL in msghdr struct from userland.

Linux manpage for recvmsg and sendmsg calls does not explicitly
mention setting msg_namelen to 0 when
msg_name passed set as NULL. When developers don't set msg_namelen
member in msghdr, it might contain garbage
value which will fail the validation check and sendmsg and recvmsg
calls from kernel will return EINVAL. This will
break old binaries and any code for which there is no access to source code.
To fix this, we set msg_namelen to 0 when msg_name is passed as NULL
from userland.

Signed-off-by: Ani Sinha <ani@aristanetworks.com>
---
 net/socket.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 95ee7d8..457be6a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1997,6 +1997,9 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
  if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
  return -EFAULT;

+ if (kmsg->msg_name == NULL)
+ kmsg->msg_namelen = 0;
+
  if (kmsg->msg_namelen < 0)
  return -EINVAL;

-- 
1.7.4.4

Re: [PATCH] net: socket: do not validate msg_namelen unless msg_name is non-NULL

[Ani Sinha]

I just resent this patch again with git send-email so that it can be
directly applied without any whitespace damage.

Subject: [PATCH 1/1] net:socket: set msg_namelen to 0 if msg_name is
passed as NULL in msghdr struct from userland.

thanks,
ani

On Fri, Sep 5, 2014 at 2:44 PM, Ani Sinha <ani@arista.com> wrote:
> On Fri, Sep 5, 2014 at 2:26 PM, Hannes Frederic Sowa
>> If you set msg_namelen = 0 if msg_name == NULL prior to the < 0 check it
>> should not trigger the return -EINVAL and also we don't run into the
>> unsafe implicit conversion case when comparing msg_namelen with the
>> result of the sizeof(). Do you see any problems with that?
>
> yes, sorry I misunderstood you. Here's the updated patch :
>
> From ea39174d4475d7def61410210613ab24a4ce0e81 Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@aristanetworks.com>
> Date: Fri, 5 Sep 2014 14:33:20 -0700
> Subject: [PATCH] net:socket: set msg_namelen to 0 if msg_name is
> passed as NULL in msghdr struct from userland.
>
> Linux manpage for recvmsg and sendmsg calls does not explicitly
> mention setting msg_namelen to 0 when
> msg_name passed set as NULL. When developers don't set msg_namelen
> member in msghdr, it might contain garbage
> value which will fail the validation check and sendmsg and recvmsg
> calls from kernel will return EINVAL. This will
> break old binaries and any code for which there is no access to source code.
> To fix this, we set msg_namelen to 0 when msg_name is passed as NULL
> from userland.
>
> Signed-off-by: Ani Sinha <ani@aristanetworks.com>
> ---
>  net/socket.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 95ee7d8..457be6a 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1997,6 +1997,9 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
>   if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
>   return -EFAULT;
>
> + if (kmsg->msg_name == NULL)
> + kmsg->msg_namelen = 0;
> +
>   if (kmsg->msg_namelen < 0)
>   return -EINVAL;
>
> --
> 1.7.4.4