| 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: <6375e778-43e8-7cf7-4ddc-19771148ecdf@alu.unizg.hr>
Date: Sat, 8 Apr 2023 11:33:22 +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. 23:08, Mirsad Goran Todorovac wrote:
> 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.
P.S.
Apparently, AFAICS, in this context:
lib/test_firmware.c:lines 842-858:
if (test_fw_config->partial)
req->rc = request_partial_firmware_into_buf
(&req->fw,
req->name,
req->dev,
test_buf,
test_fw_config->buf_size,
test_fw_config->file_offset);
else
req->rc = request_firmware_into_buf
(&req->fw,
req->name,
req->dev,
test_buf,
test_fw_config->buf_size);
if (!req->fw)
kfree(test_buf);
we call
request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf, test_fw_config->buf_size);
that calls drivers/base/firmware_loader/main.c:1035-1036:
ret = _request_firmware(firmware_p, name, device, buf, size, 0,
FW_OPT_UEVENT | FW_OPT_NOCACHE);
which calls line 814-815:
ret = _request_firmware_prepare(&fw, name, device, buf, size,
offset, opt_flags);
which calls line 748-749:
ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
offset, opt_flags);
which calls line 189:
tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags);
which does line 112-153:
static struct fw_priv *__allocate_fw_priv(const char *fw_name,
struct firmware_cache *fwc,
void *dbuf,
size_t size,
size_t offset,
u32 opt_flags)
{
struct fw_priv *fw_priv;
/* For a partial read, the buffer must be preallocated. */
if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
return NULL;
/* Only partial reads are allowed to use an offset. */
if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
return NULL;
fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
if (!fw_priv)
return NULL;
fw_priv->fw_name = kstrdup_const(fw_name, GFP_ATOMIC);
if (!fw_priv->fw_name) {
kfree(fw_priv);
return NULL;
}
kref_init(&fw_priv->ref);
fw_priv->fwc = fwc;
fw_priv->data = dbuf;
fw_priv->allocated_size = size;
fw_priv->offset = offset;
fw_priv->opt_flags = opt_flags;
fw_state_init(fw_priv);
#ifdef CONFIG_FW_LOADER_USER_HELPER
INIT_LIST_HEAD(&fw_priv->pending_list);
#endif
pr_debug("%s: fw-%s fw_priv=%p\n", __func__, fw_name, fw_priv);
return fw_priv;
}
Now, fw_priv->data is set to test_buf, fw_priv->allocated_size is set to test_fw_config->buf_size.
When release_firmware(fw) is called, it calls in line drivers/base/firmware_loader/main.c:1081:
firmware_free_data(fw) which calls lines 582-591:
/* firmware holds the ownership of pages */
static void firmware_free_data(const struct firmware *fw)
{
/* Loaded directly? */
if (!fw->priv) {
vfree(fw->data);
return;
}
free_fw_priv(fw->priv);
}
fw_priv is allocated in line 129 of drivers/base/firmware_loader/main.c:
fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
so vfree(fw->data) is not called.
free_fw_priv(fw->priv) is in lines 224-230:
void free_fw_priv(struct fw_priv *fw_priv)
{
struct firmware_cache *fwc = fw_priv->fwc;
spin_lock(&fwc->lock);
if (!kref_put(&fw_priv->ref, __free_fw_priv))
spin_unlock(&fwc->lock);
}
which calls __free_fw_priv(struct kref *ref), lines 202-222:
static void __free_fw_priv(struct kref *ref)
__releases(&fwc->lock)
{
struct fw_priv *fw_priv = to_fw_priv(ref);
struct firmware_cache *fwc = fw_priv->fwc;
pr_debug("%s: fw-%s fw_priv=%p data=%p size=%u\n",
__func__, fw_priv->fw_name, fw_priv, fw_priv->data,
(unsigned int)fw_priv->size);
list_del(&fw_priv->list);
spin_unlock(&fwc->lock);
if (fw_is_paged_buf(fw_priv))
fw_free_paged_buf(fw_priv);
else if (!fw_priv->allocated_size)
vfree(fw_priv->data);
kfree_const(fw_priv->fw_name);
kfree(fw_priv);
}
This has this construct:
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);
but as fw->priv was set to test_fw_config->buf_size with the line
141 fw_priv->data = dbuf;
142 fw_priv->allocated_size = size;
apparently vfree(fw_priv->data) is never being called for the firmware loaded
with
req-rc = request_firmware_into_buf(&req->fw, req->name, req->dev, test_buf,
test_fw_config->buf_size);
so IMHO we need to release it on the side of the caller, for this is what causes
the leaks sized test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE where
TEST_FIRMWARE_BUF_SIZE is
#define TEST_FIRMWARE_BUF_SIZE SZ_1K
that is, the actual size of the memleaks:
unreferenced object 0xffff9e011c39f000 (size 1024):
comm "test_firmware-2", pid 27646, jiffies 4302559254 (age 466.216s)
hex dump (first 32 bytes):
45 46 47 48 34 35 36 37 0a 00 00 00 00 00 00 00 EFGH4567........
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<ffffffff9390d59c>] slab_post_alloc_hook+0x8c/0x4f0
[<ffffffff93914a69>] __kmem_cache_alloc_node+0x1d9/0x2a0
[<ffffffff93883a9e>] kmalloc_trace+0x2e/0xc0
[<ffffffff93cd1760>] test_fw_run_batch_request+0x90/0x170
[<ffffffff935d901a>] kthread+0x11a/0x150
[<ffffffff93402fc9>] ret_from_fork+0x29/0x50
(71-73 of them per run of tools/testing/selftest/firmware/fw_filesystem.sh)
I hope this helps.
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