[PATCH] l2_packet: Fix valgrind uninitialised byte(s) error messages

[Jouni Malinen]

On Fri, Feb 06, 2015 at 11:07:14AM +0900, Masashi Honma wrote:
> The valgrind-3.10.0 outputs following message on Ubuntu 14.10 64bit.
> 
> ==2942== Syscall param socketcall.bind(my_addr.sa_data) points to uninitialised byte(s)
> ==2942==    at 0x5ED3577: bind (syscall-template.S:81)
> ==2942==    by 0x4AB2FE: l2_packet_init (l2_packet_linux.c:211)

This looks very strange..

> This does not occur on Ubuntu 14.10 32bit. So this looks padding problem.
> The size of struct sockaddr_ll is 20 bytes. This value is dividable by 4 but not
> 8. This patch replace the struct by struct sockaddr_storage which is designed by
> RFC 2553 to cover all types of sockaddr and have enough padding for 64bit.

I was able to reproduce this on Ubuntu 14.10 64-bit, but don't see it on
Ubuntu 14.04 64-bit which seems to be using the same version of
valgrind. gcc version is different, though.

It looks like adding just two bytes to the sockaddr_ll buffer is enough
to make valgrind not complain. I'm not sure how padding could really
cause this, though, taken into account how valgrind code for this is
implemented in pre_mem_read_sockaddr(). That implementation is not aware
of sockaddr_ll (maybe it would be a good idea to make it aware of
that..), but it seems to be checking the area following sa_family (i.e.,
sll_family in sockaddr_ll) in a way that should not go beyond the
20-byte buffer regardless of how the fields are padded. There is a note
there to point out possibility of false positives due to padding bytes,
but that would not really be applicable here since the l2_packet_linux.c
implementation clear the full buffer with memset (and well, there is not
really any padding octets in sockaddr_ll, i.e., all those 20 bytes are
within the real data fields).

> diff --git a/src/l2_packet/l2_packet_linux.c b/src/l2_packet/l2_packet_linux.c
> @@ -87,7 +87,10 @@ int l2_packet_get_own_addr(struct l2_packet_data *l2, u8 *addr)
>  int l2_packet_send(struct l2_packet_data *l2, const u8 *dst_addr, u16 proto,
>  		   const u8 *buf, size_t len)
>  {
> +	struct sockaddr_storage storage;
> +	struct sockaddr_ll *ll;

I'm not completely sure what is triggering the issue, but the code here
in l2_packet_linux.c looks just fine as-is and it would be unfortunate
if this type of workarounds are needed just to make valgrind happy. I'd
rather figure out what is the real reason behind this bogus warning and
fix that rather than change l2_packet_linux.c unless someone can point a
clear bug there. 

-- 
Jouni Malinen                                            PGP id EFC895FA