netdev - Re: [PATCH net-next v3 1/2] driver core: auxiliary bus: show auxiliary device IRQs

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: <abfa794a-e129-414a-bb5e-815eeb13131e@intel.com>
Date: Fri, 10 May 2024 15:07:09 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Shay Drori <shayd@...dia.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, <davem@...emloft.net>, <kuba@...nel.org>,
	<edumazet@...gle.com>, <gregkh@...uxfoundation.org>,
	<david.m.ertman@...el.com>
CC: <rafael@...nel.org>, <ira.weiny@...el.com>, <linux-rdma@...r.kernel.org>,
	<leon@...nel.org>, <tariqt@...dia.com>, Parav Pandit <parav@...dia.com>
Subject: Re: [PATCH net-next v3 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs

please not that v4+ is already being discussed

On 5/8/24 13:33, Shay Drori wrote:
> On 08/05/2024 12:34, Przemek Kitszel wrote:

// ...

>>> +
>>> +/* Auxiliary devices can share IRQs. Expose to user whether the 
>>> provided IRQ is
>>> + * shared or exclusive.
>>> + */
>>> +static ssize_t auxiliary_irq_mode_show(struct device *dev,
>>> +                                    struct device_attribute *attr, 
>>> char *buf)
>>> +{
>>> +     struct auxiliary_irq_info *info =
>>> +             container_of(attr, struct auxiliary_irq_info, sysfs_attr);
>>> +
>>> +     if (refcount_read(xa_load(&irqs, info->irq)) > 1)
>>
>> I didn't checked if it is possible with current implementation, but
>> please imagine a scenario where user open()'s sysfs file, then triggers
>> operation to remove irq (to call auxiliary_irq_destroy()), and only then
>> read()'s sysfs contents, what results in nullptr dereference (xa_load()
>> returning NULL). Splitting the code into two if statements would resolve
>> this issue.
> 
> the first function in auxiliary_irq_destroy() is removing the sysfs.
> I don't see how after that user can read() the sysfs...

Let me illustrate, but with my running kernel instead of your series:
# strace cat /sys/class/net/enp0s31f6/duplex 2>&1 | grep -e open -e read
yields (among others):
openat(AT_FDCWD, "/sys/class/net/enp0s31f6/duplex", O_RDONLY) = 3
read(3, "full\n", 131072)               = 5

And now imagine that other, concurrent user app or any HW event triggers
this IRQ removal (resulting with xarray entry removed (!), likely sysfs
attr refcount dropped to 0 [A], so new open()s will be declined, but
that is irrelevant).
My assumption is that, until close()d, user is free to call read() on
fd received from openat(), but it's possible that xa_load() would return
NULL (because of [A] above).

> 
>>
>>> +             return sysfs_emit(buf, "%s\n", "shared");
>>> +     else
>>> +             return sysfs_emit(buf, "%s\n", "exclusive");
>>> +}
>>> +
>>> +static void auxiliary_irq_destroy(int irq)
>>> +{
>>> +     refcount_t *ref;
>>> +
>>> +     xa_lock(&irqs);
>>> +     ref = xa_load(&irqs, irq);
>>> +     if (refcount_dec_and_test(ref)) {
>>> +             __xa_erase(&irqs, irq);
>>> +             kfree(ref);
>>> +     }
>>> +     xa_unlock(&irqs);
>>> +}
>>> +
>>> +static int auxiliary_irq_create(int irq)
>>> +{
>>> +     refcount_t *ref;
>>> +     int ret = 0;
>>> +
>>> +     mutex_lock(&irqs_lock);
>>
>> [1] xa_lock() instead ...
>>
>>> +     ref = xa_load(&irqs, irq);
>>> +     if (ref && refcount_inc_not_zero(ref))
>>> +             goto out;
>>
>> `&& refcount_inc_not_zero()` here means: leak memory and wreak havoc on
>> saturation, instead the logic should be:
>>         if (ref) {
>>                 refcount_inc(ref);
>>                 goto out;
>>         }
>>


<digression>

>> anyway allocating under a lock taken is not the best idea in general,
>> although xarray API somehow encourages this - 
> 
>> alternative is to
>> preallocate and free when not used, or some lock dance that will be easy
>> to get wrong - and that's the raison d'etre of xa_reserve() :)
> 
> I don't understand what you picture here?

Here I was digressing, sorry for not marking it clearly as that.
IMO xarray API need an extension to make this and similar use case
easier to code right. I will CC you ofc.
</digression>

> xa_reserve() can drop the lock while allocating the xa_entry, so how it 
> will help?
> 
>>
>>> +
>>> +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
>>> +     if (!ref) {
>>> +             ret = -ENOMEM;
>>> +             goto out;
>>> +     }
>>> +
>>> +     refcount_set(ref, 1);
>>> +     ret = xa_insert(&irqs, irq, ref, GFP_KERNEL);
>>
>> [2] ... then __xa_insert() here
> 
> __xa_insert() can drop the lock as well...

Thank you for pointing it to me.

Let's move future discussion on this series to your newer submissions.

// ...