| 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
| ||
|
Message-ID: <25131af2-17f2-4e3d-a11f-247cb1c4fff4@icloud.com>
Date: Mon, 19 Aug 2024 19:41:13 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>, Davidlohr Bueso
<dave@...olabs.net>, Jonathan Cameron <jonathan.cameron@...wei.com>,
Dave Jiang <dave.jiang@...el.com>,
Alison Schofield <alison.schofield@...el.com>,
Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
Dan Williams <dan.j.williams@...el.com>, Timur Tabi <timur@...nel.org>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
linux1394-devel@...ts.sourceforge.net, netdev@...r.kernel.org,
Zijun Hu <quic_zijuhu@...cinc.com>
Subject: Re: [PATCH v2 3/4] firewire: core: Prevent device_find_child() from
modifying caller's match data
On 2024/8/19 16:58, Takashi Sakamoto wrote:
>
> Hi,
>
> On 2024/8/18 22:34, Zijun Hu wrote:
>> On 2024/8/17 17:57, Takashi Sakamoto wrote:
>>> ======== 8< --------
>>>
>>> From ceaa8a986ae07865eb3fec810de330e96b6d56e2 Mon Sep 17 00:00:00 2001
>>> From: Takashi Sakamoto <o-takashi@...amocchi.jp>
>>> Date: Sat, 17 Aug 2024 17:52:53 +0900
>>> Subject: [PATCH] firewire: core: update fw_device outside of
>>> device_find_child()
>>>
>>> When detecting updates of bus topology, the data of fw_device is newly
>>> allocated and caches the content of configuration ROM from the
>>> corresponding node. Then, the tree of device is sought to find the
>>> previous data of fw_device corresponding to the node, since in IEEE 1394
>>> specification numeric node identifier could be changed dynamically every
>>> generation of bus topology. If it is found, the previous data is updated
>>> and reused, then the newly allocated data is going to be released.
>>>
>>> The above procedure is done in the call of device_find_child(), however it
>>> is a bit abusing against the intention of the helper function, since the
>>> call would not only find but also update.
>>>
>>> This commit splits the update outside of the call.
>>> ---
>>> drivers/firewire/core-device.c | 109 ++++++++++++++++-----------------
>>> 1 file changed, 54 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/firewire/core-device.c b/drivers/firewire/core-device.c
>>> index bc4c9e5a..62e8d839 100644
>>> --- a/drivers/firewire/core-device.c
>>> +++ b/drivers/firewire/core-device.c
>>> ...
>>> @@ -1038,6 +988,17 @@ int fw_device_set_broadcast_channel(struct device *dev, void *gen)
>>> return 0;
>>> }
>>>
>>> +static int compare_configuration_rom(struct device *dev, void *data)
>>> +{
>>> + const struct fw_device *old = fw_device(dev);
>>> + const u32 *config_rom = data;
>>> +
>>> + if (!is_fw_device(dev))
>>> + return 0;
>>> +
>>> + return !!memcmp(old->config_rom, config_rom, 6 * 4);
>>
>> !memcmp(old->config_rom, config_rom, 6 * 4) ?
>
> Indeed.
>
>> is this extra condition old->state == FW_DEVICE_GONE required ?
>>
>> namely, is it okay for below return ?
>> return !memcmp(old->config_rom, config_rom, 6 * 4) && old->state ==
>> FW_DEVICE_GONE
>
> If so, atomic_read() should be used, however I avoid it since the access
> to state member happens twice in in the path to reuse the instance.
>
it sounds good to not append the extra condition.
>>> +}
>>> +
>>> static void fw_device_init(struct work_struct *work)
>>> {
>>> struct fw_device *device =
>>> @@ -1071,13 +1032,51 @@ static void fw_device_init(struct work_struct *work)
>>> return;
>>> }
>>>
>>> - revived_dev = device_find_child(card->device,
>>> - device, lookup_existing_device);
>>> + // If a device was pending for deletion because its node went away but its bus info block
>>> + // and root directory header matches that of a newly discovered device, revive the
>>> + // existing fw_device. The newly allocated fw_device becomes obsolete instead.
>>> + //
>>> + // serialize config_rom access.
>>> + scoped_guard(rwsem_read, &fw_device_rwsem) {
>>> + // TODO: The cast to 'void *' could be removed if Zijun Hu's work goes well.
>>
>> may remove this TODO line since i will simply remove the cast with the
>> other patch series as shown below:
>> https://lore.kernel.org/all/20240811-const_dfc_done-v1-0-9d85e3f943cb@quicinc.com/
>
> Of course, I won't apply this patch as is. It is just a mark to hold
> your attention.
>
>>> + revived_dev = device_find_child(card->device, (void *)device->config_rom,
>>> + compare_configuration_rom);
>>> + }
>>> if (revived_dev) {
>>> - put_device(revived_dev);
>>> - fw_device_release(&device->device);
>>> + struct fw_device *found = fw_device(revived_dev);
>>>
>>> - return;
>>> + // serialize node access
>>> + guard(spinlock_irq)(&card->lock);
>>> +
>>> + if (atomic_cmpxchg(&found->state,
>>> + FW_DEVICE_GONE,
>>> + FW_DEVICE_RUNNING) == FW_DEVICE_GONE) {
>>> + struct fw_node *current_node = device->node;
>>> + struct fw_node *obsolete_node = found->node;
>>> +
>>> + device->node = obsolete_node;
>>> + device->node->data = device;
>>> + found->node = current_node;
>>> + found->node->data = found;
>>> +
>>> + found->max_speed = device->max_speed;
>>> + found->node_id = current_node->node_id;
>>> + smp_wmb(); /* update node_id before generation */
>>> + found->generation = card->generation;
>>> + found->config_rom_retries = 0;
>>> + fw_notice(card, "rediscovered device %s\n", dev_name(revived_dev));
>>> +
>>> + found->workfn = fw_device_update;
>>> + fw_schedule_device_work(found, 0);
>>> +
>>> + if (current_node == card->root_node)
>>> + fw_schedule_bm_work(card, 0);
>>> +
>>> + put_device(revived_dev);
>>> + fw_device_release(&device->device);
>>> +
>>> + return;
>>> + }
>>
>> is it okay to put_device() here as well ?
>> put_device(revived_dev);
>
> Exactly. The call of put_device() should be done when the call of
> device_find_child() returns non-NULL value.
>
> Additionally, I realize that the call of fw_device_release() under
> acquiring card->lock causes dead lock.
>
>>> }
>>>
>>> device_initialize(&device->device);
>
> Anyway, I'll post take 2 and work for its evaluation.
>
great
>
> Thanks
>
> Takashi Sakamoto
Powered by blists - more mailing lists