When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
set to 'i - 1' to prevent double-free when cleanup resources.
BUG: KASAN: double-free or invalid-free in __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
Call Trace:
kfree+0x117/0x4c0
__iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
iio_buffers_alloc_sysfs_and_mask+0x60d/0x1570 [industrialio]
__iio_device_register+0x483/0x1a30 [industrialio]
ina2xx_probe+0x625/0x980 [ina2xx_adc]
Reported-by: Hulk Robot <[email protected]>
Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO buffers")
Signed-off-by: Yang Yingliang <[email protected]>
---
drivers/iio/industrialio-buffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index a95cc2da56be..5f4bd0b73d03 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1616,7 +1616,7 @@ int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
buffer = iio_dev_opaque->attached_buffers[i];
ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
if (ret) {
- unwind_idx = i;
+ unwind_idx = i - 1;
goto error_unwind_sysfs_and_mask;
}
}
--
2.25.1
On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
<[email protected]> wrote:
>
> When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
> set to 'i - 1' to prevent double-free when cleanup resources.
>
> BUG: KASAN: double-free or invalid-free in __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
> Call Trace:
> kfree+0x117/0x4c0
> __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
> iio_buffers_alloc_sysfs_and_mask+0x60d/0x1570 [industrialio]
> __iio_device_register+0x483/0x1a30 [industrialio]
> ina2xx_probe+0x625/0x980 [ina2xx_adc]
>
Makes sense.
Thanks for the catch.
Reviewed-by: Alexandru Ardelean <[email protected]>
> Reported-by: Hulk Robot <[email protected]>
> Fixes: ee708e6baacd ("iio: buffer: introduce support for attaching more IIO buffers")
> Signed-off-by: Yang Yingliang <[email protected]>
> ---
> drivers/iio/industrialio-buffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index a95cc2da56be..5f4bd0b73d03 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1616,7 +1616,7 @@ int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev)
> buffer = iio_dev_opaque->attached_buffers[i];
> ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
> if (ret) {
> - unwind_idx = i;
> + unwind_idx = i - 1;
> goto error_unwind_sysfs_and_mask;
> }
> }
> --
> 2.25.1
>
On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
<[email protected]> wrote:
>
> On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> <[email protected]> wrote:
> >
> > When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
> > set to 'i - 1' to prevent double-free when cleanup resources.
> >
> > BUG: KASAN: double-free or invalid-free in __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
> > Call Trace:
> > kfree+0x117/0x4c0
> > __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
> > iio_buffers_alloc_sysfs_and_mask+0x60d/0x1570 [industrialio]
> > __iio_device_register+0x483/0x1a30 [industrialio]
> > ina2xx_probe+0x625/0x980 [ina2xx_adc]
> >
>
> Makes sense.
> Thanks for the catch.
>
> Reviewed-by: Alexandru Ardelean <[email protected]>
...
> > ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
> > if (ret) {
> > - unwind_idx = i;
> > + unwind_idx = i - 1;
> > goto error_unwind_sysfs_and_mask;
I prefer to see
- for (; unwind_idx >= 0; unwind_idx--) {
+ while (unwind_idx--)
instead.
> > }
> > }
--
With Best Regards,
Andy Shevchenko
On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
> On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> <[email protected]> wrote:
> >
> > On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> > <[email protected]> wrote:
> > >
> > > When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
> > > set to 'i - 1' to prevent double-free when cleanup resources.
[]
>
> I prefer to see
>
> - for (; unwind_idx >= 0; unwind_idx--) {
> + while (unwind_idx--)
Not the same code as unwind_idx would be decremented before entering
the code block.
You'd want
do {
...
} while (unwind_idx--);
On Tue, Oct 12, 2021 at 8:43 PM Joe Perches <[email protected]> wrote:
>
> On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> > <[email protected]> wrote:
> > >
> > > On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> > > <[email protected]> wrote:
> > > >
> > > > When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
> > > > set to 'i - 1' to prevent double-free when cleanup resources.
> []
> >
> > I prefer to see
> >
> > - for (; unwind_idx >= 0; unwind_idx--) {
> > + while (unwind_idx--)
>
> Not the same code as unwind_idx would be decremented before entering
> the code block.
It's kinda cryptic what you are pointing out.
What's needed additionally is to change
- unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
+ unwind_idx = i;
> You'd want
>
> do {
> ...
> } while (unwind_idx--);
Of course not. See above. The usual pattern is
while (i--)
do_clean_item(i);
--
With Best Regards,
Andy Shevchenko
On Tue, 2021-10-12 at 23:48 +0300, Andy Shevchenko wrote:
> On Tue, Oct 12, 2021 at 8:43 PM Joe Perches <[email protected]> wrote:
> >
> > On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> > > <[email protected]> wrote:
> > > >
> > > > On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> > > > <[email protected]> wrote:
> > > > >
> > > > > When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
> > > > > set to 'i - 1' to prevent double-free when cleanup resources.
> > []
> > >
> > > I prefer to see
> > >
> > > - for (; unwind_idx >= 0; unwind_idx--) {
> > > + while (unwind_idx--)
> >
> > Not the same code as unwind_idx would be decremented before entering
> > the code block.
>
> It's kinda cryptic what you are pointing out.
Not really,
> What's needed additionally is to change
>
> - unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
> + unwind_idx = i;
You left out that 'additional change' above from your reply.
> Of course not. See above. The usual pattern is
>
> ??while (i--)
> ????do_clean_item(i);
Of course, but that's not what you replied.
I was merely pointing out that your reply included a logic change
converting a loop from for to while.
cheers, Joe
On Tue, Oct 12, 2021 at 8:55 PM Joe Perches <[email protected]> wrote:
> On Tue, 2021-10-12 at 23:48 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 12, 2021 at 8:43 PM Joe Perches <[email protected]> wrote:
> > > On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> > > > <[email protected]> wrote:
> > > > > On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> > > > > <[email protected]> wrote:
...
> > > > I prefer to see
> > > >
> > > > - for (; unwind_idx >= 0; unwind_idx--) {
> > > > + while (unwind_idx--)
> > >
> > > Not the same code as unwind_idx would be decremented before entering
> > > the code block.
> >
> > It's kinda cryptic what you are pointing out.
>
> Not really,
It's. It lacks the very same "additional" words to explain what you
meant and why.
> > What's needed additionally is to change
> >
> > - unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
> > + unwind_idx = i;
>
> You left out that 'additional change' above from your reply.
Yes, that's true, but it took some time to decrypt your message.
> > Of course not. See above. The usual pattern is
> >
> > while (i--)
> > do_clean_item(i);
>
> Of course, but that's not what you replied.
> I was merely pointing out that your reply included a logic change
> converting a loop from for to while.
I expect that developers actually think about the changes they do and
double check what's proposed by reviewers. If they just copy'n'paste
whatever others propose, I wouldn't take any patch from such a
developer.
--
With Best Regards,
Andy Shevchenko
Hi,
On 2021/10/13 4:30, Andy Shevchenko wrote:
> On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> <[email protected]> wrote:
>> On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
>> <[email protected]> wrote:
>>> When __iio_buffer_alloc_sysfs_and_mask() failed, 'unwind_idx' should be
>>> set to 'i - 1' to prevent double-free when cleanup resources.
>>>
>>> BUG: KASAN: double-free or invalid-free in __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
>>> Call Trace:
>>> kfree+0x117/0x4c0
>>> __iio_buffer_free_sysfs_and_mask+0x32/0xb0 [industrialio]
>>> iio_buffers_alloc_sysfs_and_mask+0x60d/0x1570 [industrialio]
>>> __iio_device_register+0x483/0x1a30 [industrialio]
>>> ina2xx_probe+0x625/0x980 [ina2xx_adc]
>>>
>> Makes sense.
>> Thanks for the catch.
>>
>> Reviewed-by: Alexandru Ardelean <[email protected]>
> ...
>
>>> ret = __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev, i);
>>> if (ret) {
>>> - unwind_idx = i;
>>> + unwind_idx = i - 1;
>>> goto error_unwind_sysfs_and_mask;
> I prefer to see
>
> - for (; unwind_idx >= 0; unwind_idx--) {
> + while (unwind_idx--)
>
> instead.
With using while loop, 'unwind_idx =
iio_dev_opaque->attached_buffers_cnt - 1;' need
be changed.
I think my change is clear and simple, do I need resend a new version
with using while loop ?
Thanks,
Yang
>
>>> }
>>> }
Hi,
On 2021/10/13 4:58, Andy Shevchenko wrote:
> On Tue, Oct 12, 2021 at 8:55 PM Joe Perches <[email protected]> wrote:
>> On Tue, 2021-10-12 at 23:48 +0300, Andy Shevchenko wrote:
>>> On Tue, Oct 12, 2021 at 8:43 PM Joe Perches <[email protected]> wrote:
>>>> On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
>>>>> On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
>>>>> <[email protected]> wrote:
>>>>>> On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
>>>>>> <[email protected]> wrote:
> ...
>
>>>>> I prefer to see
>>>>>
>>>>> - for (; unwind_idx >= 0; unwind_idx--) {
>>>>> + while (unwind_idx--)
>>>> Not the same code as unwind_idx would be decremented before entering
>>>> the code block.
>>> It's kinda cryptic what you are pointing out.
>> Not really,
> It's. It lacks the very same "additional" words to explain what you
> meant and why.
>
>>> What's needed additionally is to change
>>>
>>> - unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
>>> + unwind_idx = i;
>> You left out that 'additional change' above from your reply.
> Yes, that's true, but it took some time to decrypt your message.
>
>>> Of course not. See above. The usual pattern is
>>>
>>> while (i--)
>>> do_clean_item(i);
>> Of course, but that's not what you replied.
>> I was merely pointing out that your reply included a logic change
>> converting a loop from for to while.
> I expect that developers actually think about the changes they do and
> double check what's proposed by reviewers. If they just copy'n'paste
> whatever others propose, I wouldn't take any patch from such a
> developer.
I think in alloc path is using for loop, and in error/free path also
using for loop is better to read the code.
>
On Wed, Oct 13, 2021 at 12:17 PM Yang Yingliang
<[email protected]> wrote:
> On 2021/10/13 4:58, Andy Shevchenko wrote:
> > On Tue, Oct 12, 2021 at 8:55 PM Joe Perches <[email protected]> wrote:
> >> On Tue, 2021-10-12 at 23:48 +0300, Andy Shevchenko wrote:
> >>> On Tue, Oct 12, 2021 at 8:43 PM Joe Perches <[email protected]> wrote:
> >>>> On Tue, 2021-10-12 at 23:30 +0300, Andy Shevchenko wrote:
> >>>>> On Tue, Oct 12, 2021 at 2:37 PM Alexandru Ardelean
> >>>>> <[email protected]> wrote:
> >>>>>> On Tue, Oct 12, 2021 at 12:18 PM Yang Yingliang
> >>>>>> <[email protected]> wrote:
> > ...
> >
> >>>>> I prefer to see
> >>>>>
> >>>>> - for (; unwind_idx >= 0; unwind_idx--) {
> >>>>> + while (unwind_idx--)
> >>>> Not the same code as unwind_idx would be decremented before entering
> >>>> the code block.
> >>> It's kinda cryptic what you are pointing out.
> >> Not really,
> > It's. It lacks the very same "additional" words to explain what you
> > meant and why.
> >
> >>> What's needed additionally is to change
> >>>
> >>> - unwind_idx = iio_dev_opaque->attached_buffers_cnt - 1;
> >>> + unwind_idx = i;
> >> You left out that 'additional change' above from your reply.
> > Yes, that's true, but it took some time to decrypt your message.
> >
> >>> Of course not. See above. The usual pattern is
> >>>
> >>> while (i--)
> >>> do_clean_item(i);
> >> Of course, but that's not what you replied.
> >> I was merely pointing out that your reply included a logic change
> >> converting a loop from for to while.
> > I expect that developers actually think about the changes they do and
> > double check what's proposed by reviewers. If they just copy'n'paste
> > whatever others propose, I wouldn't take any patch from such a
> > developer.
> I think in alloc path is using for loop, and in error/free path also
> using for loop is better to read the code.
I don't think so.
while(idx--)
is kinda idiom which is really easy to read.
I could send a v2 on your behalf.
--
With Best Regards,
Andy Shevchenko
On Wed, Oct 13, 2021 at 12:28 PM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Oct 13, 2021 at 12:17 PM Yang Yingliang
> <[email protected]> wrote:
> I could send a v2 on your behalf.
Looking into code further I think that your patch is good as is, see
v2 I have just sent.
--
With Best Regards,
Andy Shevchenko