linux-kernel - Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure field ordering

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]

Date:   Wed, 1 Feb 2017 10:43:10 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Peter Meerwald-Stadler <pmeerw@...erw.net>
CC:     <linux-kernel@...r.kernel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Alison Schofield <amsfield22@...il.com>,
        Gregor Boirie <gregor.boirie@...rot.com>,
        Sanchayan Maity <maitysanchayan@...il.com>,
        Ken Lin <ken.lin@...antech.com>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 1/2] iio: pressure: mpl3115: do not rely on structure
 field ordering

On 2017-02-01 10:31, Peter Meerwald-Stadler wrote:
> 
>>>> Fixes a regression triggered by a change in the layout of
>>>> struct iio_chan_spec, but the real bug is in the driver which assumed
>>>> a specific structure layout in the first place.
> 
>>> what do you mean by 'driver which assumed a specific structure'?
>>
>> Look again, the two bits are not OR:ed together as implied by the
>> indentation. There is a comma between them, which put the ..._SCALE
>> bit in the next field. That next field was .info_mask_shared_by_type
>> before the patch adding the available attribute that triggered the
>> regression and .info_mask_separate_available after it.
> 
> wow, now that you say it :)
> 
> since I wrote these drivers I can assure you that this is just a typo, use
> of the 'specific structure' is a coincident
> 
> maybe adding your explanation regarding not ORing to the patch description
> would be a good idea
> 
> I was confused by the change from 
> .info_mask_separate to .info_mask_shared_by_type
> a fix could have just changed
> -  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> 			BIT(IIO_CHAN_INFO_SCALE),
> +  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 			BIT(IIO_CHAN_INFO_SCALE),
> as originally intended

I considered that option, but the code in mpl3115_read_raw (and
mpl115_read_raw for that matter) return constants fro these values which
to me indicated that they were not "separate" and as that would also be
the change which replicated the exact behavior from before the regression
I went with that. But I don't care either way, so I can re-spin if you
want me to? (But don't blame me if that regresses in some other
interesting way).

Cheers,
peda

>> Cheers,
>> peda
>>
>>> thanks, p.
>>>
>>>> diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
>>>> index cc3f84139157..525644a7442d 100644
>>>> --- a/drivers/iio/pressure/mpl3115.c
>>>> +++ b/drivers/iio/pressure/mpl3115.c
>>>> @@ -190,7 +190,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>>>  	{
>>>>  		.type = IIO_PRESSURE,
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> -			BIT(IIO_CHAN_INFO_SCALE),
>>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.scan_index = 0,
>>>>  		.scan_type = {
>>>>  			.sign = 'u',
>>>> @@ -203,7 +203,7 @@ static const struct iio_chan_spec mpl3115_channels[] = {
>>>>  	{
>>>>  		.type = IIO_TEMP,
>>>>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>>> -			BIT(IIO_CHAN_INFO_SCALE),
>>>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>>>>  		.scan_index = 1,
>>>>  		.scan_type = {
>>>>  			.sign = 's',
>>>>
>>>
>>
>