From: Vasiliy Kulikov <[email protected]>
Date: Sun, 31 Oct 2010 20:10:32 +0300
> Structure sockaddr_tipc is copied to userland with padding bytes after
> "id" field in union field "name" unitialized. It leads to leaking of
> contents of kernel stack memory. We have to initialize them to zero.
>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
Applied.
Patches #1 and #2 were given feedback which I need you to integrate
and submit new patches based upon, thanks.
On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:
> From: Vasiliy Kulikov <[email protected]>
> Date: Sun, 31 Oct 2010 20:10:32 +0300
>
> > Structure sockaddr_tipc is copied to userland with padding bytes after
> > "id" field in union field "name" unitialized. It leads to leaking of
> > contents of kernel stack memory. We have to initialize them to zero.
> >
> > Signed-off-by: Vasiliy Kulikov <[email protected]>
>
> Applied.
>
> Patches #1 and #2 were given feedback which I need you to integrate
> and submit new patches based upon, thanks.
About #2:
I still think that this:
if (dev)
strncpy(uaddr->sa_data, dev->name, 14);
else
memset(uaddr->sa_data, 0, 14);
is better than this:
memset(uaddr->sa_data, 0, 14);
dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex);
if (dev)
strlcpy(uaddr->sa_data, dev->name, 15);
Doesn't it? Explicitly filling with zero on the same "if" level is
slightly easier to read and understand.
--
Vasiliy
Am 09.11.2010 21:33, schrieb Vasiliy Kulikov:
> On Tue, Nov 09, 2010 at 09:26 -0800, David Miller wrote:
>> From: Vasiliy Kulikov <[email protected]>
>> Date: Sun, 31 Oct 2010 20:10:32 +0300
>>
>>> Structure sockaddr_tipc is copied to userland with padding bytes after
>>> "id" field in union field "name" unitialized. It leads to leaking of
>>> contents of kernel stack memory. We have to initialize them to zero.
>>>
>>> Signed-off-by: Vasiliy Kulikov <[email protected]>
>>
>> Applied.
>>
>> Patches #1 and #2 were given feedback which I need you to integrate
>> and submit new patches based upon, thanks.
>
> About #2:
>
> I still think that this:
>
> if (dev)
> strncpy(uaddr->sa_data, dev->name, 14);
> else
> memset(uaddr->sa_data, 0, 14);
>
> is better than this:
>
> memset(uaddr->sa_data, 0, 14);
> dev = dev_get_by_index_rcu(sock_net(sk), pkt_sk(sk)->ifindex);
> if (dev)
> strlcpy(uaddr->sa_data, dev->name, 15);
>
> Doesn't it? Explicitly filling with zero on the same "if" level is
> slightly easier to read and understand.
>
no problem with me, since i came up with the idea a simple explanation:
IMHO the pattern clear/if/copy is more robust
NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15.
re,
wh
On Wed, Nov 10, 2010 at 12:58 +0100, walter harms wrote:
> NTL the core problem was that sizeof sa_data is 14 while dev->name is IFNAMESZ=15.
With this code it is NOT a bug because the output buffer is much bigger
than 14 (128 bytes). I think it was just designed to overflow 14 bytes,
assign sa_data[14] = 0 and ignore it (lack of snprintf() those days?).
Anywhere else sa_data[14] = ... is a bug.
--
Vasiliy