linux-kernel - Re: [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in fw_free_paged

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:   Mon, 28 Mar 2022 12:09:42 -0600
From:   Russ Weight <russell.h.weight@...el.com>
To:     Tom Rix <trix@...hat.com>, <mcgrof@...nel.org>,
        <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
        <linux-kernel@...r.kernel.org>
CC:     <lgoncalv@...hat.com>, <yilun.xu@...el.com>, <hao.wu@...el.com>,
        <matthew.gerlach@...el.com>, <basheer.ahmed.muddebihal@...el.com>,
        <tianfei.zhang@...el.com>
Subject: Re: [RESEND PATCH v1 1/8] firmware_loader: Clear data and size in
 fw_free_paged_buf

Hi Tom,

On 3/28/22 06:27, Tom Rix wrote:
>
> On 3/23/22 4:33 PM, Russ Weight wrote:
>> The fw_free_paged_buf() function resets the paged buffer information in
>> the fw_priv data structure. Additionally, clear the data and size members
>> of fw_priv in order to facilitate the reuse of fw_priv. This is being
>> done in preparation for enabling userspace to initiate multiple firmware
>> uploads using this sysfs interface.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
>> ---
>> v1:
>>    - No change from RFC patch
>> ---
>>   drivers/base/firmware_loader/main.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
>> index 94d1789a233e..2cc11d93753a 100644
>> --- a/drivers/base/firmware_loader/main.c
>> +++ b/drivers/base/firmware_loader/main.c
>> @@ -253,6 +253,8 @@ void fw_free_paged_buf(struct fw_priv *fw_priv)
>
> Why isn't a vfree needed or realloc done?

The free and realloc support was present prior to my changes. The page
buffer support was designed such that if a firmware write was cancelled, the
existing fw_priv structure could be re-used for another write in the context
of the same firmware upload. However, there was no prior case for completing
a write and then reusing the fw_priv structure for subsequent firmware writes;
fw_priv previously had a one-time use. The changes I have made are to enable
the re-use of the fw_priv structure.

Initially, fw_priv->data is NULL. The "realloc" functionality happens during
the write of the data binary attribute here:

https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L426

The fw_priv->data pointer remains NULL until all data is written and the
user writes '0' to the loading attribute. The fw_priv->data pointer is set in
fw_map_paged_buf() which is called here:

https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/fallback.c#L274

In the unmodified code, the fw_priv->data pointer is never cleared. My changes
reset the pointer to NULL after the memory is released so that the fw_priv can
be resused.

The new firmware-upload happens in the context of a kernel worker thread and the work
function is fw_upload_main(). At the end of fw_upload_main(), fw_free_paged_buf()
is called to do the free. This is the function that is being modified by the lines
below. This function calls "__free_page(fw_priv->pages[i])" in a loop to free the
memory pages. It also calls "vunmap(fw_priv->data)" to free the virtual mapping.
You can see the unmodified implementation of this function here:

https://github.com/torvalds/linux/blob/ae085d7f9365de7da27ab5c0d16b12d51ea7fca9/drivers/base/firmware_loader/main.c#L241

- Russ

>
> Tom
>
>>       fw_priv->pages = NULL;
>>       fw_priv->page_array_size = 0;
>>       fw_priv->nr_pages = 0;
>> +    fw_priv->data = NULL;
>> +    fw_priv->size = 0;
>>   }
>>     int fw_grow_paged_buf(struct fw_priv *fw_priv, int pages_needed)
>