net: uninitialized loopback addr leaks to userspace

net: uninitialized loopback addr leaks to userspace

[Vegard Nossum]

Hi,

It seems that loopback's hardware address is never initialized by the
kernel. So if userspace attempts to read this address before it has
been set, the kernel will return some uninitialized data (only 6
bytes, though). This can be demonstrated by creating a new network
namespace (CLONE_NEWNET), which creates a new loopback device, then
call ioctl() with SIOCGIFHWADDR on "lo". If this is done in a loop,
with some background load, or by running multiple instances, random
data will start to show up in the returned address.

[  406.750329] WARNING: kmemcheck: Caught 16-bit read from
uninitialized memory (ffff880007220974)
[  406.753555] 18a2d7060088ffff18a2d7060088ffff00000000010000000100000003000000
[  406.758862]  i i i i i i i i i i i i i i i i i u u u u u u u u u u u u u u u
[  406.766224]                                          ^
[  406.768792] Modules linked in:
[  406.770416] Pid: 757, comm: ifconfig Not tainted
2.6.30-rc7-next-20090529 #404
[  406.772876] RIP: 0010:[<ffffffff80664789>]  [<ffffffff80664789>]
dev_ioctl+0x5d9/0x600
[  406.804677]  [<ffffffff8064ff75>] sock_ioctl+0x95/0x2a0
[  406.807242]  [<ffffffff802c35eb>] vfs_ioctl+0x1b/0x70
[  406.809348]  [<ffffffff802c36fa>] do_vfs_ioctl+0x8a/0x570
[  406.811419]  [<ffffffff802c3c79>] sys_ioctl+0x99/0xa0
[  406.813400]  [<ffffffff802f3941>] dev_ifsioc+0x81/0x2f0
[  406.815424]  [<ffffffff802f454d>] compat_sys_ioctl+0xed/0x3c0
[  406.817596]  [<ffffffff8022d476>] cstar_dispatch+0x7/0x26
[  406.819978]  [<ffffffffffffffff>] 0xffffffffffffffff

This is the code that triggers the warning, in net/core/dev.c, around line 4150:

    memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
        min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));

So it's dev->dev_addr that is the pointer to the uninitialized data.

I didn't know how to fix it.


Vegard

Re: net: uninitialized loopback addr leaks to userspace

[John Dykstra]

On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
> It seems that loopback's hardware address is never initialized by the
> kernel. So if userspace attempts to read this address before it has
> been set, the kernel will return some uninitialized data (only 6
> bytes, though).

Thank you for the report, Vegard.

I've been unable to reproduce the problem you describe, using
2.6-30-rc8, this test program and a couple of kernel builds for system
load:

------------------------------------------------------------------
#define REPEAT_COUNT 10000

int childTask() {
	struct ifreq ifreq;
	int fd;
	unsigned char allBits;

	fd = socket(AF_INET, SOCK_DGRAM, 0);
	if (fd < 0){
		printf("Error %s from socket()\n", strerror(errno));
		_exit(-1);
	}

	strncpy(ifreq.ifr_name, "lo", sizeof("lo"));
	if (ioctl (fd, SIOCGIFHWADDR, &ifreq) < 0){
		printf("Error %s from ioctl(SIOCGIFHWADDR) for %s.\n", strerror(errno), ifreq.ifr_name);
		_exit(-1);
	}
	
	allBits = ifreq.ifr_hwaddr.sa_data[0] |
			ifreq.ifr_hwaddr.sa_data[1] |
			ifreq.ifr_hwaddr.sa_data[2] |
			ifreq.ifr_hwaddr.sa_data[3] |
			ifreq.ifr_hwaddr.sa_data[4] |
			ifreq.ifr_hwaddr.sa_data[5];

	if (allBits != 0)
		printf("Device %s -> Ethernet %02x:%02x:%02x:%02x:%02x:%02x\n", ifreq.ifr_name,
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[0],
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[1],
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[2],
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[3],
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[4],
			(int) ((unsigned char *) &ifreq.ifr_hwaddr.sa_data)[5]);
}


int main(int argc, char *argv[]) {
   	void **child_stack;
	int pid, i, status;

	child_stack = (void **) malloc(16384);

	for (i = 0; i < REPEAT_COUNT; i++){

		pid = clone(childTask, child_stack, CLONE_NEWNET, NULL);
		if (pid < 0){
			printf("Error %s from clone()\n", strerror(errno));
			_exit(-1);
		}
		
		pid = waitpid(pid, &status, __WCLONE);
		if (pid < 0){
			printf("Error %s from waitpid()\n", strerror(errno));
			_exit(-1);
		}
	}   
      
	return 0;
}
------------------------------------------------------------------

Looking at the kernel code, it appears that all bytes of struct
net_device, including the L2 address, are initialized to zeros at
interface creation time.

Can you spot a difference between your test procedures and mine that
would enable me to reproduce the problem?

  --  John

Re: net: uninitialized loopback addr leaks to userspace

[Vegard Nossum]

2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
>> It seems that loopback's hardware address is never initialized by the
>> kernel. So if userspace attempts to read this address before it has
>> been set, the kernel will return some uninitialized data (only 6
>> bytes, though).
>
> Thank you for the report, Vegard.
>
> I've been unable to reproduce the problem you describe, using
> 2.6-30-rc8, this test program and a couple of kernel builds for system
> load:
[...]
> ------------------------------------------------------------------
>
> Looking at the kernel code, it appears that all bytes of struct
> net_device, including the L2 address, are initialized to zeros at
> interface creation time.
>
> Can you spot a difference between your test procedures and mine that
> would enable me to reproduce the problem?

Hi,

I just tried your test program on a linux-next kernel, it works beautifully :-)

(I made one change: The stack grows downwards on x86, so I think you
should put child_stack + 16386 as the stack to clone()?)

As I wrote in reply to Stephen Hemminger, this problem seems to be
caused by a particular patch in linux-next:

commit f001fde5eadd915f4858d22ed70d7040f48767cf
Author: Jiri Pirko <jpirko@redhat.com>
Date:   Tue May 5 02:48:28 2009 +0000

   net: introduce a list of device addresses dev_addr_list (v6)

Thanks for testing.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

[PATCH net-next-2.6] net: loopback device dev->addr_len fix

[Eric Dumazet]

Vegard Nossum a écrit :
> 2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
>> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
>>> It seems that loopback's hardware address is never initialized by the
>>> kernel. So if userspace attempts to read this address before it has
>>> been set, the kernel will return some uninitialized data (only 6
>>> bytes, though).
>> Thank you for the report, Vegard.
>>
>> I've been unable to reproduce the problem you describe, using
>> 2.6-30-rc8, this test program and a couple of kernel builds for system
>> load:
> [...]
>> ------------------------------------------------------------------
>>
>> Looking at the kernel code, it appears that all bytes of struct
>> net_device, including the L2 address, are initialized to zeros at
>> interface creation time.
>>
>> Can you spot a difference between your test procedures and mine that
>> would enable me to reproduce the problem?
> 
> Hi,
> 
> I just tried your test program on a linux-next kernel, it works beautifully :-)
> 
> (I made one change: The stack grows downwards on x86, so I think you
> should put child_stack + 16386 as the stack to clone()?)
> 
> As I wrote in reply to Stephen Hemminger, this problem seems to be
> caused by a particular patch in linux-next:
> 
> commit f001fde5eadd915f4858d22ed70d7040f48767cf
> Author: Jiri Pirko <jpirko@redhat.com>
> Date:   Tue May 5 02:48:28 2009 +0000
> 
>    net: introduce a list of device addresses dev_addr_list (v6)
> 

I believe following patch should fix this problem.

Thank you

[PATCH net-next-2.6] net: loopback device dev->addr_len fix

commit f001fde5eadd915f4858d22ed70d7040f48767cf 
(net: introduce a list of device addresses dev_addr_list (v6))
added one regression Vegard Nossum found in its testings.

loopback device doesnt have a hw address, we should set its
dev->addr_len to 0, not ETH_ALEN.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index da472c6..40ded4e 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -166,7 +166,7 @@ static void loopback_setup(struct net_device *dev)
 {
 	dev->mtu		= (16 * 1024) + 20 + 20 + 12;
 	dev->hard_header_len	= ETH_HLEN;	/* 14	*/
-	dev->addr_len		= ETH_ALEN;	/* 6	*/
+	dev->addr_len		= 0;
 	dev->tx_queue_len	= 0;
 	dev->type		= ARPHRD_LOOPBACK;	/* 0x0001*/
 	dev->flags		= IFF_LOOPBACK;

[PATCH net-next-2.6] net: dev_addr_init() fix

[Eric Dumazet]

Eric Dumazet a écrit :
> Vegard Nossum a écrit :
>> 2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
>>> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
>>>> It seems that loopback's hardware address is never initialized by the
>>>> kernel. So if userspace attempts to read this address before it has
>>>> been set, the kernel will return some uninitialized data (only 6
>>>> bytes, though).
>>> Thank you for the report, Vegard.
>>>
>>> I've been unable to reproduce the problem you describe, using
>>> 2.6-30-rc8, this test program and a couple of kernel builds for system
>>> load:
>> [...]
>>> ------------------------------------------------------------------
>>>
>>> Looking at the kernel code, it appears that all bytes of struct
>>> net_device, including the L2 address, are initialized to zeros at
>>> interface creation time.
>>>
>>> Can you spot a difference between your test procedures and mine that
>>> would enable me to reproduce the problem?
>> Hi,
>>
>> I just tried your test program on a linux-next kernel, it works beautifully :-)
>>
>> (I made one change: The stack grows downwards on x86, so I think you
>> should put child_stack + 16386 as the stack to clone()?)
>>
>> As I wrote in reply to Stephen Hemminger, this problem seems to be
>> caused by a particular patch in linux-next:
>>
>> commit f001fde5eadd915f4858d22ed70d7040f48767cf
>> Author: Jiri Pirko <jpirko@redhat.com>
>> Date:   Tue May 5 02:48:28 2009 +0000
>>
>>    net: introduce a list of device addresses dev_addr_list (v6)
>>
> 
> I believe following patch should fix this problem.
> 
> Thank you
> 
> [PATCH net-next-2.6] net: loopback device dev->addr_len fix
> 
> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
> (net: introduce a list of device addresses dev_addr_list (v6))
> added one regression Vegard Nossum found in its testings.
> 
> loopback device doesnt have a hw address, we should set its
> dev->addr_len to 0, not ETH_ALEN.
> 
> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Oh well, following is probably even more appropriate

[PATCH net-next-2.6] net: dev_addr_init() fix

commit f001fde5eadd915f4858d22ed70d7040f48767cf 
(net: introduce a list of device addresses dev_addr_list (v6))
added one regression Vegard Nossum found in its testings.

dev_addr_init() incorrectly uses sizeof() operator

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/core/dev.c b/net/core/dev.c
index 1f38401..65387d9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3655,8 +3655,8 @@ static int dev_addr_init(struct net_device *dev)
 	/* rtnl_mutex must be held here */
 
 	INIT_LIST_HEAD(&dev->dev_addr_list);
-	memset(addr, 0, sizeof(*addr));
-	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(*addr),
+	memset(addr, 0, sizeof(addr));
+	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(addr),
 			    NETDEV_HW_ADDR_T_LAN);
 	if (!err) {
 		/*

Re: [PATCH net-next-2.6] net: dev_addr_init() fix

[Jiri Pirko]

Mon, Jun 08, 2009 at 02:13:32PM CEST, dada1@cosmosbay.com wrote:
>Eric Dumazet a écrit :
>> Vegard Nossum a écrit :
>>> 2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
>>>> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
>>>>> It seems that loopback's hardware address is never initialized by the
>>>>> kernel. So if userspace attempts to read this address before it has
>>>>> been set, the kernel will return some uninitialized data (only 6
>>>>> bytes, though).
>>>> Thank you for the report, Vegard.
>>>>
>>>> I've been unable to reproduce the problem you describe, using
>>>> 2.6-30-rc8, this test program and a couple of kernel builds for system
>>>> load:
>>> [...]
>>>> ------------------------------------------------------------------
>>>>
>>>> Looking at the kernel code, it appears that all bytes of struct
>>>> net_device, including the L2 address, are initialized to zeros at
>>>> interface creation time.
>>>>
>>>> Can you spot a difference between your test procedures and mine that
>>>> would enable me to reproduce the problem?
>>> Hi,
>>>
>>> I just tried your test program on a linux-next kernel, it works beautifully :-)
>>>
>>> (I made one change: The stack grows downwards on x86, so I think you
>>> should put child_stack + 16386 as the stack to clone()?)
>>>
>>> As I wrote in reply to Stephen Hemminger, this problem seems to be
>>> caused by a particular patch in linux-next:
>>>
>>> commit f001fde5eadd915f4858d22ed70d7040f48767cf
>>> Author: Jiri Pirko <jpirko@redhat.com>
>>> Date:   Tue May 5 02:48:28 2009 +0000
>>>
>>>    net: introduce a list of device addresses dev_addr_list (v6)
>>>
>> 
>> I believe following patch should fix this problem.
>> 
>> Thank you
>> 
>> [PATCH net-next-2.6] net: loopback device dev->addr_len fix
>> 
>> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
>> (net: introduce a list of device addresses dev_addr_list (v6))
>> added one regression Vegard Nossum found in its testings.
>> 
>> loopback device doesnt have a hw address, we should set its
>> dev->addr_len to 0, not ETH_ALEN.
>> 
>> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

oops, sorry for this...

Signed-off-by: Jiri Pirko <jpirko@redhat.com>

>
>Oh well, following is probably even more appropriate
>
>[PATCH net-next-2.6] net: dev_addr_init() fix
>
>commit f001fde5eadd915f4858d22ed70d7040f48767cf 
>(net: introduce a list of device addresses dev_addr_list (v6))
>added one regression Vegard Nossum found in its testings.
>
>dev_addr_init() incorrectly uses sizeof() operator
>
>Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>---
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 1f38401..65387d9 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -3655,8 +3655,8 @@ static int dev_addr_init(struct net_device *dev)
> 	/* rtnl_mutex must be held here */
> 
> 	INIT_LIST_HEAD(&dev->dev_addr_list);
>-	memset(addr, 0, sizeof(*addr));
>-	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(*addr),
>+	memset(addr, 0, sizeof(addr));
>+	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(addr),
> 			    NETDEV_HW_ADDR_T_LAN);
> 	if (!err) {
> 		/*
>

Re: [PATCH net-next-2.6] net: dev_addr_init() fix

[Ingo Molnar]

* Eric Dumazet <dada1@cosmosbay.com> wrote:

> Eric Dumazet a écrit :
> > Vegard Nossum a écrit :
> >> 2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
> >>> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
> >>>> It seems that loopback's hardware address is never initialized by the
> >>>> kernel. So if userspace attempts to read this address before it has
> >>>> been set, the kernel will return some uninitialized data (only 6
> >>>> bytes, though).
> >>> Thank you for the report, Vegard.
> >>>
> >>> I've been unable to reproduce the problem you describe, using
> >>> 2.6-30-rc8, this test program and a couple of kernel builds for system
> >>> load:
> >> [...]
> >>> ------------------------------------------------------------------
> >>>
> >>> Looking at the kernel code, it appears that all bytes of struct
> >>> net_device, including the L2 address, are initialized to zeros at
> >>> interface creation time.
> >>>
> >>> Can you spot a difference between your test procedures and mine that
> >>> would enable me to reproduce the problem?
> >> Hi,
> >>
> >> I just tried your test program on a linux-next kernel, it works beautifully :-)
> >>
> >> (I made one change: The stack grows downwards on x86, so I think you
> >> should put child_stack + 16386 as the stack to clone()?)
> >>
> >> As I wrote in reply to Stephen Hemminger, this problem seems to be
> >> caused by a particular patch in linux-next:
> >>
> >> commit f001fde5eadd915f4858d22ed70d7040f48767cf
> >> Author: Jiri Pirko <jpirko@redhat.com>
> >> Date:   Tue May 5 02:48:28 2009 +0000
> >>
> >>    net: introduce a list of device addresses dev_addr_list (v6)
> >>
> > 
> > I believe following patch should fix this problem.
> > 
> > Thank you
> > 
> > [PATCH net-next-2.6] net: loopback device dev->addr_len fix
> > 
> > commit f001fde5eadd915f4858d22ed70d7040f48767cf 
> > (net: introduce a list of device addresses dev_addr_list (v6))
> > added one regression Vegard Nossum found in its testings.
> > 
> > loopback device doesnt have a hw address, we should set its
> > dev->addr_len to 0, not ETH_ALEN.
> > 
> > Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Oh well, following is probably even more appropriate
> 
> [PATCH net-next-2.6] net: dev_addr_init() fix
> 
> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
> (net: introduce a list of device addresses dev_addr_list (v6))
> added one regression Vegard Nossum found in its testings.
> 
> dev_addr_init() incorrectly uses sizeof() operator
> 
> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Could you please put the word 'kmemcheck' somewhere into the 
changelog, to make git-grepping and historic comparisons easier?

Thanks,

	Ingo

Re: [PATCH net-next-2.6] net: dev_addr_init() fix

[Eric Dumazet]

Ingo Molnar a écrit :
> * Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
>> Eric Dumazet a écrit :
>>> Vegard Nossum a écrit :
>>>> 2009/6/7 John Dykstra <john.dykstra1@gmail.com>:
>>>>> On Sat, 2009-05-30 at 22:23 +0200, Vegard Nossum wrote:
>>>>>> It seems that loopback's hardware address is never initialized by the
>>>>>> kernel. So if userspace attempts to read this address before it has
>>>>>> been set, the kernel will return some uninitialized data (only 6
>>>>>> bytes, though).
>>>>> Thank you for the report, Vegard.
>>>>>
>>>>> I've been unable to reproduce the problem you describe, using
>>>>> 2.6-30-rc8, this test program and a couple of kernel builds for system
>>>>> load:
>>>> [...]
>>>>> ------------------------------------------------------------------
>>>>>
>>>>> Looking at the kernel code, it appears that all bytes of struct
>>>>> net_device, including the L2 address, are initialized to zeros at
>>>>> interface creation time.
>>>>>
>>>>> Can you spot a difference between your test procedures and mine that
>>>>> would enable me to reproduce the problem?
>>>> Hi,
>>>>
>>>> I just tried your test program on a linux-next kernel, it works beautifully :-)
>>>>
>>>> (I made one change: The stack grows downwards on x86, so I think you
>>>> should put child_stack + 16386 as the stack to clone()?)
>>>>
>>>> As I wrote in reply to Stephen Hemminger, this problem seems to be
>>>> caused by a particular patch in linux-next:
>>>>
>>>> commit f001fde5eadd915f4858d22ed70d7040f48767cf
>>>> Author: Jiri Pirko <jpirko@redhat.com>
>>>> Date:   Tue May 5 02:48:28 2009 +0000
>>>>
>>>>    net: introduce a list of device addresses dev_addr_list (v6)
>>>>
>>> I believe following patch should fix this problem.
>>>
>>> Thank you
>>>
>>> [PATCH net-next-2.6] net: loopback device dev->addr_len fix
>>>
>>> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
>>> (net: introduce a list of device addresses dev_addr_list (v6))
>>> added one regression Vegard Nossum found in its testings.
>>>
>>> loopback device doesnt have a hw address, we should set its
>>> dev->addr_len to 0, not ETH_ALEN.
>>>
>>> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
>>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Oh well, following is probably even more appropriate
>>
>> [PATCH net-next-2.6] net: dev_addr_init() fix
>>
>> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
>> (net: introduce a list of device addresses dev_addr_list (v6))
>> added one regression Vegard Nossum found in its testings.
>>
>> dev_addr_init() incorrectly uses sizeof() operator
>>
>> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Could you please put the word 'kmemcheck' somewhere into the 
> changelog, to make git-grepping and historic comparisons easier?
> 

Sure I can do that, giving me opportunity to use my current email address,
since dada1@cosmosbay.com will disappear shortly.

Thank you

[PATCH net-next-2.6] net: dev_addr_init() fix

commit f001fde5eadd915f4858d22ed70d7040f48767cf 
(net: introduce a list of device addresses dev_addr_list (v6))
added one regression Vegard Nossum found in its testings.

With kmemcheck help, Vegard found some uninitialized memory
was read and reported to user, potentialy leaking kernel data.
( thread can be found on http://lkml.org/lkml/2009/5/30/177 )

dev_addr_init() incorrectly uses sizeof() operator. We were
initializing one byte instead of MAX_ADDR_LEN bytes.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Jiri Pirko <jpirko@redhat.com>
---

diff --git a/net/core/dev.c b/net/core/dev.c
index 1f38401..65387d9 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3655,8 +3655,8 @@ static int dev_addr_init(struct net_device *dev)
 	/* rtnl_mutex must be held here */
 
 	INIT_LIST_HEAD(&dev->dev_addr_list);
-	memset(addr, 0, sizeof(*addr));
-	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(*addr),
+	memset(addr, 0, sizeof(addr));
+	err = __hw_addr_add(&dev->dev_addr_list, NULL, addr, sizeof(addr),
 			    NETDEV_HW_ADDR_T_LAN);
 	if (!err) {
 		/*

Re: [PATCH net-next-2.6] net: dev_addr_init() fix

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 08 Jun 2009 15:49:24 +0200

> [PATCH net-next-2.6] net: dev_addr_init() fix
> 
> commit f001fde5eadd915f4858d22ed70d7040f48767cf 
> (net: introduce a list of device addresses dev_addr_list (v6))
> added one regression Vegard Nossum found in its testings.
> 
> With kmemcheck help, Vegard found some uninitialized memory
> was read and reported to user, potentialy leaking kernel data.
> ( thread can be found on http://lkml.org/lkml/2009/5/30/177 )
> 
> dev_addr_init() incorrectly uses sizeof() operator. We were
> initializing one byte instead of MAX_ADDR_LEN bytes.
> 
> Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> Acked-by: Jiri Pirko <jpirko@redhat.com>

Applied, thanks.

Re: net: uninitialized loopback addr leaks to userspace

[Stephen Hemminger]

On Sat, 30 May 2009 22:23:24 +0200
Vegard Nossum <vegard.nossum@gmail.com> wrote:

> Hi,
> 
> It seems that loopback's hardware address is never initialized by the
> kernel. So if userspace attempts to read this address before it has
> been set, the kernel will return some uninitialized data (only 6
> bytes, though). This can be demonstrated by creating a new network
> namespace (CLONE_NEWNET), which creates a new loopback device, then
> call ioctl() with SIOCGIFHWADDR on "lo". If this is done in a loop,
> with some background load, or by running multiple instances, random
> data will start to show up in the returned address.
> 
> [  406.750329] WARNING: kmemcheck: Caught 16-bit read from
> uninitialized memory (ffff880007220974)
> [  406.753555] 18a2d7060088ffff18a2d7060088ffff00000000010000000100000003000000
> [  406.758862]  i i i i i i i i i i i i i i i i i u u u u u u u u u u u u u u u
> [  406.766224]                                          ^
> [  406.768792] Modules linked in:
> [  406.770416] Pid: 757, comm: ifconfig Not tainted
> 2.6.30-rc7-next-20090529 #404
> [  406.772876] RIP: 0010:[<ffffffff80664789>]  [<ffffffff80664789>]
> dev_ioctl+0x5d9/0x600
> [  406.804677]  [<ffffffff8064ff75>] sock_ioctl+0x95/0x2a0
> [  406.807242]  [<ffffffff802c35eb>] vfs_ioctl+0x1b/0x70
> [  406.809348]  [<ffffffff802c36fa>] do_vfs_ioctl+0x8a/0x570
> [  406.811419]  [<ffffffff802c3c79>] sys_ioctl+0x99/0xa0
> [  406.813400]  [<ffffffff802f3941>] dev_ifsioc+0x81/0x2f0
> [  406.815424]  [<ffffffff802f454d>] compat_sys_ioctl+0xed/0x3c0
> [  406.817596]  [<ffffffff8022d476>] cstar_dispatch+0x7/0x26
> [  406.819978]  [<ffffffffffffffff>] 0xffffffffffffffff
> 
> This is the code that triggers the warning, in net/core/dev.c, around line 4150:
> 
>     memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
>         min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
> 
> So it's dev->dev_addr that is the pointer to the uninitialized data.
> 
> I didn't know how to fix it.
> 


The whole dev structure is zeroed in alloc_netdev(), kmemcheck
is giving bogus warning.
-- 

Re: net: uninitialized loopback addr leaks to userspace

[Vegard Nossum]

2009/6/8 Stephen Hemminger <shemminger@vyatta.com>:
> On Sat, 30 May 2009 22:23:24 +0200
> Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
>> Hi,
>>
>> It seems that loopback's hardware address is never initialized by the
>> kernel. So if userspace attempts to read this address before it has
>> been set, the kernel will return some uninitialized data (only 6
>> bytes, though). This can be demonstrated by creating a new network
>> namespace (CLONE_NEWNET), which creates a new loopback device, then
>> call ioctl() with SIOCGIFHWADDR on "lo". If this is done in a loop,
>> with some background load, or by running multiple instances, random
>> data will start to show up in the returned address.
>>
>> [  406.750329] WARNING: kmemcheck: Caught 16-bit read from
>> uninitialized memory (ffff880007220974)
>> [  406.753555] 18a2d7060088ffff18a2d7060088ffff00000000010000000100000003000000
>> [  406.758862]  i i i i i i i i i i i i i i i i i u u u u u u u u u u u u u u u
>> [  406.766224]                                          ^
>> [  406.768792] Modules linked in:
>> [  406.770416] Pid: 757, comm: ifconfig Not tainted
>> 2.6.30-rc7-next-20090529 #404
>> [  406.772876] RIP: 0010:[<ffffffff80664789>]  [<ffffffff80664789>]
>> dev_ioctl+0x5d9/0x600
>> [  406.804677]  [<ffffffff8064ff75>] sock_ioctl+0x95/0x2a0
>> [  406.807242]  [<ffffffff802c35eb>] vfs_ioctl+0x1b/0x70
>> [  406.809348]  [<ffffffff802c36fa>] do_vfs_ioctl+0x8a/0x570
>> [  406.811419]  [<ffffffff802c3c79>] sys_ioctl+0x99/0xa0
>> [  406.813400]  [<ffffffff802f3941>] dev_ifsioc+0x81/0x2f0
>> [  406.815424]  [<ffffffff802f454d>] compat_sys_ioctl+0xed/0x3c0
>> [  406.817596]  [<ffffffff8022d476>] cstar_dispatch+0x7/0x26
>> [  406.819978]  [<ffffffffffffffff>] 0xffffffffffffffff
>>
>> This is the code that triggers the warning, in net/core/dev.c, around line 4150:
>>
>>     memcpy(ifr->ifr_hwaddr.sa_data, dev->dev_addr,
>>         min(sizeof ifr->ifr_hwaddr.sa_data, (size_t) dev->addr_len));
>>
>> So it's dev->dev_addr that is the pointer to the uninitialized data.
>>
>> I didn't know how to fix it.
>>
>
>
> The whole dev structure is zeroed in alloc_netdev(), kmemcheck
> is giving bogus warning.

Hi -- and sorry for being unclear. If I hadn't been sure that this was
a real error, I would have said so (or not reported it at all).

I investigated it now, and as can be seen in the report above, I am
using a -next kernel. It seems that the error was introduced in:

commit f001fde5eadd915f4858d22ed70d7040f48767cf
Author: Jiri Pirko <jpirko@redhat.com>
Date:   Tue May 5 02:48:28 2009 +0000

    net: introduce a list of device addresses dev_addr_list (v6)

So the error does not, as you say, exist in mainline Linux, but it's
not a bogus warning either :-)

Adding to Cc.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036