Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <751a87c9-a4aa-4e06-1d12-1e2b1a3487de@gmail.com>
Date:   Fri, 6 Oct 2023 14:10:16 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Content-Language: en-US, en-GB
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
References: <ZRvjuZaQWdZw1U1I@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi>
 <20231005163026.2c7707de@jic23-huawei>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH] iio: sanity check available_scan_masks array
In-Reply-To: <20231005163026.2c7707de@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Again Jonathan.

On 10/5/23 18:30, Jonathan Cameron wrote:
> On Tue, 3 Oct 2023 12:49:45 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> When IIO goes through the available scan masks in order to select the
>> best suiting one, it will just accept the first listed subset of channels
>> which meets the user's requirements. If driver lists a mask which is a
>> subset of some of the masks previously in the array of
>> avaliable_scan_masks, then the latter one will never be selected.
>>
>> Add a warning if driver registers masks which can't be used due to the
>> available_scan_masks-array ordering.
>>
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Hi Matti
> 
> Thanks for doing this.  A few comments inline + maybe we need to think
> about a unit test for the matching code. I feel we aren't pushing the
> corners of that in any drivers so far so it might bite us later.
> 
> Still that's a job for another day.
> 
> Jonathan
> 
>>
>> ---
>> The change was suggested by Jonathan here:
>> https://lore.kernel.org/lkml/20230924170726.41443502@jic23-huawei/
>> ---
>>   drivers/iio/industrialio-core.c | 57 +++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index c77745b594bd..d4f37f4eeec0 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -1896,6 +1896,53 @@ static int iio_check_extended_name(const struct iio_dev *indio_dev)

...

>> +
>> +	for (num_masks = 0; *av_masks; num_masks++)
> 
> I think we can't just check *av_masks - need bitmap_empty() as first
> long might be 0 but could be bits set in the next one.
> 
>> +		av_masks += longs_per_mask;

I did switch this to:
+       for (num_masks = 0; !bitmap_empty(av_masks, masklength); 
num_masks++)
+               av_masks += longs_per_mask;

but this kind of freaks me out.

I think in kernel we see two ways of constructing and passing arrays to 
frameworks. One is creating a NULL terminated array, the other being an 
array which size is given. The available_scan_masks is using the first 
approach.

The array represents bitmasks, which are thought to be of arbitrary 
length. The type of array items is longs though. When building an arry 
like this, it is easy to just do:

unsigned long masks[] = {
	mask1_hi,
	mask1_lo,
	mask2_hi,
	mask2_lo,
	...
	maskN_lo,
	/* sentinel */
	0
}

(By the way, I've always hated that 'sentinel' comment as it - in my 
opinion - is not worth adding. I think the meaning of 0 should be 
obvious, but here I just added it to alleviate the problem).

Here, if I'm not mistaken, the check I implemented would go reading out 
of the array bounds.

Knowing how easy it would be slip the above array past my reviewing eyes 
- I find this scary. And ugly part of this is that we can't detect this 
in the iio-core side, because we have no way of knowing how big the 
array and sentinel are. What makes this worse is that the core does:

for (i = 0; i < indio_dev->num_channels; i++)
                         ml = max(ml, channels[i].scan_index + 1);
                 indio_dev->masklength = ml;

so, masklength may not be what was set in driver.

I did quick and dirty grep for "_scan_mask\[" in iio directory and 
didn't spot any bigger than a few channels masks. Still, this makes me 
worried.

BTW: I did also:

Author: Matti Vaittinen <mazziesaccount@gmail.com>
Date:   Fri Oct 6 13:53:11 2023 +0300

     iio: buffer: use bitmap_empty() to find last mask

     When IIO buffer code is scanning the array of available masks for
     matching the user's enable request to channel configuration 
supported by
     driver, the code uses a 'check for long 0' as indication of last mask.
     This does not work right for channel masks greater than BITS_PER_LONG.

     Use bitmap_empty() to find the last element in available_scan_masks
     array.

     Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
     ---
     NOTE: This is potentially hazardous change. Please, don't pick without
     thorough check and understanding.

diff --git a/drivers/iio/industrialio-buffer.c 
b/drivers/iio/industrialio-buffer.c
index 176d31d9f9d8..1e59afddcf9a 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -413,7 +413,7 @@ static const unsigned long 
*iio_scan_mask_match(const unsigned long *av_masks,
  {
         if (bitmap_empty(mask, masklength))
                 return NULL;
-       while (*av_masks) {
+       while (!bitmap_empty(av_masks, masklength)) {
                 if (strict) {
                         if (bitmap_equal(mask, av_masks, masklength))
                                 return av_masks;

but this is just as fragile - for obvious reasons.

One way around this would be to have the first bit in the long always 
set for a valid mask - and take this into account when going through the 
masks. It's probably somewhat more confusing than current code though - 
but it would allow using just a single long (with all - or  at least 
first - bits zero to indicate end of masks).

Other option I see is to just error out if available_scan_masks array is 
given with larger than one 'long' wide masks and worry things when this 
breaks.

Anyways, I don't like using bitmap_empty() for array of bitmaps which 
may be longer than BITS_PER_LONG unless we can sanity check the size of 
the array...

How do you feel about this?

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~