Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fa82b641-365c-c1ca-1fec-1826df63db64@alu.unizg.hr>
Date:   Tue, 28 Mar 2023 12:33:30 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [BUG] selftests/firmware: copious kernel memory leaks in
 test_fw_run_batch_request()
Content-Language: en-US, hr
To:     Dan Carpenter <error27@gmail.com>,
        Luis Chamberlain <mcgrof@kernel.org>
Cc:     linux-kselftest@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Russ Weight <russell.h.weight@intel.com>,
        Takashi Iwai <tiwai@suse.de>,
        Tianfei zhang <tianfei.zhang@intel.com>,
        Shuah Khan <shuah@kernel.org>,
        Colin Ian King <colin.i.king@gmail.com>,
        Randy Dunlap <rdunlap@infradead.org>
References: <97e284be-5018-9d18-feb2-7ec4b08c06fd@alu.unizg.hr>
 <26fd581a-1b9f-4960-8457-61d725511cee@kili.mountain>
From:   Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
In-Reply-To: <26fd581a-1b9f-4960-8457-61d725511cee@kili.mountain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi, Dan,

On 3/28/23 12:06, Dan Carpenter wrote:
> On Tue, Mar 28, 2023 at 11:23:00AM +0200, Mirsad Todorovac wrote:
>> The leaks are in chunks of 1024 bytes (+ overhead), but so far I could not
>> reproduce w/o root privileges, as tests refuse to run as unprivileged user.
>> (This is not the proof of non-existence of an unprivileged automated exploit
>> that would exhaust the kernel memory at approx. rate 4 MB/hour on our setup.
>>
>> This would mean about 96 MB / day or 3 GB / month (of kernel memory).
> 
> This is firmware testing stuff.  In the real world people aren't going
> to run their test scripts in a loop for days.

Thank you for making that clear.

> There is no security implications.  This is root only.  Also if the
> user could load firmware then that would be the headline.  Once someone
> is can already load firmware then who cares if they leak 100MB per day?

Yes, this is correct, but I just don't like leaks even in the userland programs.
But that might be just me ...

IMHO the purpose of the tests is to find and fix bugs. There are probably
more critical issues, but pick seemed manageable.

> It looks like if you call trigger_batched_requests_store() twice in a
> row then it will leak memory.  Definitely test_fw_config->reqs is leaked.
> That's different from what the bug report is complaining about, but the
> point is that there are some obvious leaks.  It looks like you're
> supposed to call trigger_batched_requests_store() in between runs?
> 
> There are other races like config_num_requests_store() should hold the
> mutex over the call to test_dev_config_update_u8() instead of dropping
> and retaking it.

Please consider the scope of the void *test_buf in lines 836-859 and whether the
fact that test_buf is not kfree()-ed on (req->fw != NULL) and its going out of the
scope affects this issue.

I saw there is an additional race condition involved since the exact count of leaks
is not always the same (not deterministic), but I could not figure that out by myself.

Thank you again very much for your quick reply.

BTW, I can confirm that the leak still exists in 6.3.0-rc4-00025-g3a93e40326c8
build.

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

"Something is quickly approaching ... Will it be friends with me?"