linux-kernel - Re: endianness and sparse warnings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 1 Sep 2008 11:23:48 +0200 (CEST)
From:	Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.com>
To:	Harvey Harrison <harvey.harrison@...il.com>
cc:	Linux Kernel Development <linux-kernel@...r.kernel.org>
Subject: Re: endianness and sparse warnings

On Fri, 29 Aug 2008, Harvey Harrison wrote:
> On Fri, 2008-08-29 at 16:54 +0200, Geert Uytterhoeven wrote:
> > With `make  C=1 CF="-D__CHECK_ENDIAN__"', you can let sparse check for bad
> > handling of endian-annotated data.
> > 
> > Unfortunately several of the accessors for endian-annotated data do not cause
> > sparse warnings.
> 
> I'll try and give some background on why the unaligned versions are implemented
> this way.
> 
> The get_unaligned helpers were meant to replace two kinds of use (using le16 as
> an example)
> 
> char *ptr;
> 
> 1 - le16_to_cpu(get_unaligned((__le16 *)ptr))
> 2 - u16 val = ptr[0] | (ptr[1] << 8)
> 
> The places where 1 was replaced with the unaligned helpers would have been fine
> with an annotated version as it already had the cast to a proper type.
> 
> The places where 2 was replaced would have required a new cast to __le16 *.
> 
> Lots of places that were using 2 are drivers that have some data area pointed
> to by a char * and they are grabbing values from there at known offsets,
> for these users, the need for extra casting was quite ugly and it was known
> exactly how many bytes and in what endianness you are reading as it is
> right in the function name so I thought it would be ok to omit the annotation
> on the parameter.
> 
> u16 foo, bar;
> char *my_data;
> 
> foo = get_unaligned_le16((__le16 *)my_data); /* if unaligned helpers were annotated */
> bar = get_unaligned_le16(my_data);	/* current version */

Indeed, if you have a void/char *, this works, and there's not much annotation
you can do here.

But please consider this case:

    struct xxx {
	u8 a;
	__le16 b;
    } __attribute__ ((packed));

    struct xxx *p;
    u16 foo = get_unaligned_le16(&p->b);

Here you could have typechecking.
Note that a `get_unaligned_le()' that handles different sizes automatically
would work.

> > Summarized:
> >   - [bl]e{16,32,64}_to_cpu() is OK
> >   - [bl]e{16,32,64}_to_cpup() (aka get_aligned_[bl]e{16,32,64}() ;-) is OK
> >   - get_unaligned_[bl]e{16,32,64} is not OK
> >   - __get_unaligned_[bl]e() is partially OK, as long as you don't use it on
> >     non-annotated data, but
> >       o it's meant for internal use only
> >       o it incorrectly causes sparse warnings when assigning the resulting
> >         value to a non-annotated variable
> 
> Almost... __get_unaligned_le16 etc are _never_ to be used...as some arches
> choose to use memmove-based implementations, and on arches where unaligned
> access is OK, they don't exist _at_all_.

So perhaps we want a public get_unaligned_[bl]e() for the case above?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@...ycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010