Message-ID: <65bac38f-7934-4e79-0efc-43973dad0349@alu.unizg.hr>
Date:   Fri, 7 Apr 2023 23:08:17 +0200
From:   Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To:     Dan Carpenter <error27@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Russ Weight <russell.h.weight@...el.com>,
        linux-kernel@...r.kernel.org, Luis Chamberlain <mcgrof@...nel.org>,
        Tianfei zhang <tianfei.zhang@...el.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        Zhengchao Shao <shaozhengchao@...wei.com>,
        Colin Ian King <colin.i.king@...il.com>,
        Takashi Iwai <tiwai@...e.de>
Subject: Re: [PATCH v3 1/2] test_firmware: Fix some racing conditions in
 test_fw_config locking.

On 07. 04. 2023. 11:03, Dan Carpenter wrote:
> On Fri, Apr 07, 2023 at 10:24:24AM +0200, Mirsad Goran Todorovac wrote:
>>
>> Hi Mr. Carpenter,
>>
>> Thank you for your review.
>>
>> I will proceed according to your guidelines and issue the next version of the
>> patch set.
>>
>> But I cannot promise it will be before the holidays - I do not want to make
>> the gods angry either ;-)
>>
> 
> There is never a rush.
> 
>> I cannot promise to try smart macros or inline functions with smart function
>> parameters just yet.
>>
> 
> Don't worry about that.  It just seemed like you were working towards
> a more general purpose infrastructure.  It's just a clean up.
> 
>> I would consider the real success if I hunt down the remaining leak and races
>> in this driver. Despite being considered a less important one.
>>
>> As you have previously asserted, it is not a real security issue with a CVE,
>> however, for completeness sake I would like to see these problems fixed.
> 
> That's great.  If you get bored and feel like giving up then just send
> PATCH 2/2 by itself because that one could be merged as is.

Hi Mr. Carpenter,

Actually, I do not like to give up easily :-)

I seem to be onto something:

In drivers/base/firmware_loader/main.c:

 202 static void __free_fw_priv(struct kref *ref)
 203         __releases(&fwc->lock)
 204 {
 205         struct fw_priv *fw_priv = to_fw_priv(ref);
 206         struct firmware_cache *fwc = fw_priv->fwc;
 207 
 208         pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
 209                  __func__, fw_priv->fw_name, fw_priv, fw_priv->data,
 210                  (unsigned int)fw_priv->size);
 211 
 212         list_del(&fw_priv->list);
 213         spin_unlock(&fwc->lock);
 214 
 215         if (fw_is_paged_buf(fw_priv))
 216                 fw_free_paged_buf(fw_priv);
 217         else if (!fw_priv->allocated_size)
 218                 vfree(fw_priv->data);
 219 
 220         kfree_const(fw_priv->fw_name);
 221         kfree(fw_priv);
 222 }

This deallocates fw_priv->data only if fpriv->allocated_size == 0

 217         else if (!fw_priv->allocated_size)
 218                 vfree(fw_priv->data);

However, this function:

 112 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 113                                           struct firmware_cache *fwc,
 114                                           void *dbuf,
 115                                           size_t size,
 116                                           size_t offset,
 117                                           u32 opt_flags)
 118 {
 119         struct fw_priv *fw_priv;
 120 
 121         /* For a partial read, the buffer must be preallocated. */
 122         if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
 123                 return NULL;
 124 
 125         /* Only partial reads are allowed to use an offset. */
 126         if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
 127                 return NULL;
 128 
 129         fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
 130         if (!fw_priv)
 131                 return NULL;
 132 
 133         fw_priv->fw_name = kstrdup_const(fw_name, GFP_ATOMIC);
 134         if (!fw_priv->fw_name) {
 135                 kfree(fw_priv);
 136                 return NULL;
 137         }
 138 
 139         kref_init(&fw_priv->ref);
 140         fw_priv->fwc = fwc;
 141         fw_priv->data = dbuf;
 142         fw_priv->allocated_size = size;
 143         fw_priv->offset = offset;
 144         fw_priv->opt_flags = opt_flags;
 145         fw_state_init(fw_priv);
 146 #ifdef CONFIG_FW_LOADER_USER_HELPER
 147         INIT_LIST_HEAD(&fw_priv->pending_list);
 148 #endif
 149 
 150         pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
 151 
 152         return fw_priv;
 153 }

Has both set:

 141         fw_priv->data = dbuf;
 142         fw_priv->allocated_size = size;

I suspect this could be the source of the leak?

size in passed all the way down from

request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
			  struct device *device, void *buf, size_t size)

It is sized test_buf = kzalloc(TEST_FIRMWARE_BUF_SIZE, GFP_KERNEL); which is

#define TEST_FIRMWARE_BUF_SIZE	SZ_1K

(the exact size of the leak: 1024 bytes)

I did not dare to fix this, because in other contexts as xz_uncompress this
test allocated_size is used with different semantics, and I am not sure what
is the right way to fix this:

 357         if (!fw_priv->allocated_size)
 358                 fw_priv->data = out_buf;

so I would break this case.

Possibly, the way of allocation and allocated_size could be separated?

I did not expect the fix to go that deep into the kernel proper?

Just to give you a progress report.

I might even come up with a fix attempt, but not yet a formal patch I suppose.

Best regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
 
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union

"I see something approaching fast ... Will it be friends with me?"

Powered by blists - more mailing lists