linux-kernel - Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <xa1teh71iz3a.fsf@mina86.com>
Date:	Thu, 31 Oct 2013 10:11:53 +0100
From:	Michal Nazarewicz <mina86@...a86.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Evgeniy Polyakov <zbr@...emap.net>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers: w1: make w1_slave::flags long to avoid casts

On Wed, Oct 30 2013, Andrew Morton wrote:
> On Sat, 26 Oct 2013 12:56:11 +0100 Michal Nazarewicz <mpn@...gle.com> wrote:
>
>> From: Michal Nazarewicz <mina86@...a86.com>
>> 
>> Changing flags field of the w1_slave to unsigned long may on
>> some architectures increase the size of the structure, but
>> otherwise makes the code more kosher as casting is avoided
>> and *_bit family of calls do not attempt to operate on an
>> entity of bigger size than realy is available.
>> 
>> The current behaviour does not introduce any bugs (since any
>> bytes past flags field are preserved)
>
> hm, what does this mean....
>
>> --- a/drivers/w1/w1.c
>> +++ b/drivers/w1/w1.c
>> @@ -709,7 +709,7 @@ static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn)
>>  
>>  	sl->owner = THIS_MODULE;
>>  	sl->master = dev;
>> -	set_bit(W1_SLAVE_ACTIVE, (long *)&sl->flags);
>> +	set_bit(W1_SLAVE_ACTIVE, &sl->flags);
>
> ...  I'd have though that running this code on little-endian 64-bit
> would result in a scribble over ...
>
>> --- a/drivers/w1/w1.h
>> +++ b/drivers/w1/w1.h
>> @@ -67,8 +67,8 @@ struct w1_slave
>>  	struct w1_reg_num	reg_num;
>>  	atomic_t		refcnt;
>>  	u8			rom[9];
>> -	u32			flags;
>>  	int			ttl;
>
> ... w1_slave.ttl?

Now that I look at documentation, I think you are correct, but the
problem is on big-endian 64-bit architectures.  The fix is still
valid, but the commit message not so much.  Something along the
lines of the following would be better:

-------- >8 --------------------------------------------------------
drivers: w1: make w1_slave::flags long to avoid memory corruption

On architectures where long is more then 32 bits, modifying a 32-bit
field with set_bit (and other atomic bit operations) may cause bytes
following the field to by modified.

Because the endianness of the bits within a field is the native
endianness of the CPU[1], on big-endian machines, bit number zero is
in the last byte of the field.

Therefore, “set_bit(0, ptr)” on a 64-bit big-endian machine is
roughly equivalent to “((char *)ptr)[7] |= 1”, and since w1 driver
uses a 32-bit field for holding the flags, this causes bytes beyond
the field to be modified.

[1] From Documentation/atomic_ops.txt:

    Native atomic bit operations are defined to operate on objects
    aligned to the size of an "unsigned long" C data type, and are
    least of that size.  The endianness of the bits within each
    "unsigned long" are the native endianness of the cpu.
-------- >8 --------------------------------------------------------

>> +	unsigned long		flags;
>>  
>>  	struct w1_master	*master;
>>  	struct w1_family	*family;

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@...gle.com>--<xmpp:mina86@...ber.org>--ooO--(_)--Ooo--


Download attachment "signature.asc" of type "application/pgp-signature" (836 bytes)