Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[f6bvp@free]

Hi Mathias,

Thank you for the copy of the thread and sorry for the second post.
It is good to know that someone is taking care of this nice piece of 
network code.

Bernard Pidoux


On Sun, Apr 21, 2013 at 7:39 PM, f6bvp@free <f6bvp@free.fr> wrote:
> Hi,
>
> According to the proximity of NetRom and Rose codes I looked at af_rose.c
> and it seems that similarly sockaddr_rose structure is let uninitialized in
> rose_recvmsg().
>
> Then, would you consider the following patch interesting to be committed ?
>
> --- a/net/rose/af_rose.c        2013-04-17 07:11:28.000000000 +0200
> +++ b/net/rose/af_rose.c        2013-04-21 17:26:06.914967897 +0200
> @@ -1257,6 +1257,7 @@ static int rose_recvmsg(struct kiocb *io
>         skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
>
>         if (srose != NULL) {
> +               memset(srose, 0, sizeof(*srose));
>                 srose->srose_family = AF_ROSE;
>                 srose->srose_addr   = rose->dest_addr;
>                 srose->srose_call   = rose->dest_call;
>
>

Thanks, but something more complete is already in Linus tree
(sizeof(*srose) is not enough):

commit 4a184233f21645cf0b719366210ed445d1024d72
Author: Mathias Krause <minipli@googlemail.com>
Date:   Sun Apr 7 01:51:59 2013 +0000

     rose: fix info leak via msg_name in rose_recvmsg()

     The code in rose_recvmsg() does not initialize all of the members of
     struct sockaddr_rose/full_sockaddr_rose when filling the sockaddr info.
     Nor does it initialize the padding bytes of the structure inserted by
     the compiler for alignment. This will lead to leaking uninitialized
     kernel stack bytes in net/socket.c.

     Fix the issue by initializing the memory used for sockaddr info with
     memset(0).

     Cc: Ralf Baechle <ralf@linux-mips.org>
     Signed-off-by: Mathias Krause <minipli@googlemail.com>
     Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index cf68e6e..9c83474 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -1253,6 +1253,7 @@ static int rose_recvmsg(struct kiocb *iocb, struct 
socket
         skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);

         if (srose != NULL) {
+               memset(srose, 0, msg->msg_namelen);
                 srose->srose_family = AF_ROSE;
                 srose->srose_addr   = rose->dest_addr;
                 srose->srose_call   = rose->dest_call;

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[f6bvp@free]

Hi,

According to the proximity of NetRom and Rose codes I looked at af_rose.c
and it seems that similarly sockaddr_rose structure is let uninitialized 
in rose_recvmsg().

Then, would you consider the following patch interesting to be committed ?

--- a/net/rose/af_rose.c        2013-04-17 07:11:28.000000000 +0200
+++ b/net/rose/af_rose.c        2013-04-21 17:26:06.914967897 +0200
@@ -1257,6 +1257,7 @@ static int rose_recvmsg(struct kiocb *io
         skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);

         if (srose != NULL) {
+               memset(srose, 0, sizeof(*srose));
                 srose->srose_family = AF_ROSE;
                 srose->srose_addr   = rose->dest_addr;
                 srose->srose_call   = rose->dest_call;


Bernard Pidoux

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[Mathias Krause]

On Sun, Apr 21, 2013 at 7:56 PM, f6bvp@free <f6bvp@free.fr> wrote:
> Hi,
>
> According to the proximity of NetRom and Rose codes I looked at af_rose.c
> and it seems that similarly sockaddr_rose structure is let uninitialized in
> rose_recvmsg().
>
> Then, would you consider the following patch interesting to be committed ?
>
> --- a/net/rose/af_rose.c        2013-04-17 07:11:28.000000000 +0200
> +++ b/net/rose/af_rose.c        2013-04-21 17:26:06.914967897 +0200
> @@ -1257,6 +1257,7 @@ static int rose_recvmsg(struct kiocb *io
>         skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);
>
>         if (srose != NULL) {
> +               memset(srose, 0, sizeof(*srose));
>                 srose->srose_family = AF_ROSE;
>                 srose->srose_addr   = rose->dest_addr;
>                 srose->srose_call   = rose->dest_call;
>

Thanks, but something more complete is already in Linus tree
(sizeof(*srose) is not enough):

commit 4a184233f21645cf0b719366210ed445d1024d72
Author: Mathias Krause <minipli@googlemail.com>
Date:   Sun Apr 7 01:51:59 2013 +0000

    rose: fix info leak via msg_name in rose_recvmsg()

    The code in rose_recvmsg() does not initialize all of the members of
    struct sockaddr_rose/full_sockaddr_rose when filling the sockaddr info.
    Nor does it initialize the padding bytes of the structure inserted by
    the compiler for alignment. This will lead to leaking uninitialized
    kernel stack bytes in net/socket.c.

    Fix the issue by initializing the memory used for sockaddr info with
    memset(0).

    Cc: Ralf Baechle <ralf@linux-mips.org>
    Signed-off-by: Mathias Krause <minipli@googlemail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index cf68e6e..9c83474 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -1253,6 +1253,7 @@ static int rose_recvmsg(struct kiocb *iocb, struct socket
        skb_copy_datagram_iovec(skb, 0, msg->msg_iov, copied);

        if (srose != NULL) {
+               memset(srose, 0, msg->msg_namelen);
                srose->srose_family = AF_ROSE;
                srose->srose_addr   = rose->dest_addr;
                srose->srose_call   = rose->dest_call;

[PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[Wei Yongjun]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

sizeof() when applied to a pointer typed expression gives the size of the
pointer, not that of the pointed data.
Introduced by commit 3ce5ef(netrom: fix info leak via msg_name in nr_recvmsg)

Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
 net/netrom/af_netrom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 7fcb307..103bd70 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -1173,7 +1173,7 @@ static int nr_recvmsg(struct kiocb *iocb, struct socket *sock,
 	}
 
 	if (sax != NULL) {
-		memset(sax, 0, sizeof(sax));
+		memset(sax, 0, sizeof(*sax));
 		sax->sax25_family = AF_NETROM;
 		skb_copy_from_linear_data_offset(skb, 7, sax->sax25_call.ax25_call,
 			      AX25_ADDR_LEN);

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[David Miller]

From: Wei Yongjun <weiyj.lk@gmail.com>
Date: Tue, 9 Apr 2013 10:07:19 +0800

> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> sizeof() when applied to a pointer typed expression gives the size of the
> pointer, not that of the pointed data.
> Introduced by commit 3ce5ef(netrom: fix info leak via msg_name in nr_recvmsg)
> 
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Any particular reason you're:

1) Targetting the wrong tree.  This was a bug fix that went into
   'net', therefore targetting -next is not correct.  This fix
   need to go into 'net'.

2) Not even bothering to CC: the person whose change you are
   correcting?

Anyways, applied to 'net', thanks.

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[Wei Yongjun]

On 04/09/2013 10:49 AM, David Miller wrote:
> From: Wei Yongjun <weiyj.lk@gmail.com>
> Date: Tue, 9 Apr 2013 10:07:19 +0800
>
>> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>>
>> sizeof() when applied to a pointer typed expression gives the size of the
>> pointer, not that of the pointed data.
>> Introduced by commit 3ce5ef(netrom: fix info leak via msg_name in nr_recvmsg)
>>
>> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> Any particular reason you're:
>
> 1) Targetting the wrong tree.  This was a bug fix that went into
>    'net', therefore targetting -next is not correct.  This fix
>    need to go into 'net'.


I found this from the linux-next.git tree, but this commit is not exists
at linux.git tree, and I forgot to check net.git, sorry for that.
I will pay more attention next time.


>
> 2) Not even bothering to CC: the person whose change you are
>    correcting?
>
> Anyways, applied to 'net', thanks.
>
>
>

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[Hannes Frederic Sowa]

On Tue, Apr 09, 2013 at 10:07:19AM +0800, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> 
> sizeof() when applied to a pointer typed expression gives the size of the
> pointer, not that of the pointed data.
> Introduced by commit 3ce5ef(netrom: fix info leak via msg_name in nr_recvmsg)

I think it would be a good idea to enable -Wsizeof-pointer-memaccess
(gcc 4.8 feature) by default. While enabled there were no additional
warnings when I tested it some weeks ago. I will look into  this.

Re: [PATCH -next] netrom: fix invalid use of sizeof in nr_recvmsg()

[Mathias Krause]

On Tue, Apr 9, 2013 at 4:07 AM, Wei Yongjun <weiyj.lk@gmail.com> wrote:
>
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> sizeof() when applied to a pointer typed expression gives the size of the
> pointer, not that of the pointed data.
> Introduced by commit 3ce5ef(netrom: fix info leak via msg_name in nr_recvmsg)
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
>  net/netrom/af_netrom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
> index 7fcb307..103bd70 100644
> --- a/net/netrom/af_netrom.c
> +++ b/net/netrom/af_netrom.c
> @@ -1173,7 +1173,7 @@ static int nr_recvmsg(struct kiocb *iocb, struct socket *sock,
>         }
>
>         if (sax != NULL) {
> -               memset(sax, 0, sizeof(sax));
> +               memset(sax, 0, sizeof(*sax));
>                 sax->sax25_family = AF_NETROM;
>                 skb_copy_from_linear_data_offset(skb, 7, sax->sax25_call.ax25_call,
>                               AX25_ADDR_LEN);
>

Thanks for fixing this nasty little typo! My keyboard must be broken
as sizeof(*sax) was the intended change in the first place. ;)

Acked-by: Mathias Krause <minipli@googlemail.com>