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

[Eric Dumazet <eric.dumazet@gmail.com>]

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) {
 		/*