netdev - Re: [PATCH] chcr_ktls: fix a possible null-pointer dereference in chcr_ktls_dev

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] [day] [month] [year] [list]

Message-ID: <CADm8TemexVr3gcuhKJ9M4PLDg2bF85nhT17a8J1uf_qqj_fKiQ@mail.gmail.com>
Date: Tue, 5 Nov 2024 07:32:31 +0800
From: Tuo Li <islituo@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: ayush.sawal@...lsio.com, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	almasrymina@...gle.com, dtatulea@...dia.com, jacob.e.keller@...el.com, 
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org, baijiaju1990@...il.com
Subject: Re: [PATCH] chcr_ktls: fix a possible null-pointer dereference in chcr_ktls_dev_add()

Hi Simon,

Thanks for your reply! It is very helpful.
Any further feedback will be appreciated.

Thank you very much!

Sincerely,
Tuo Li




On 2024/11/5 0:07, Simon Horman wrote:
> On Thu, Oct 31, 2024 at 12:23:52AM +1100, Tuo Li wrote:
>> There is a possible null-pointer dereference related to the wait-completion
>> synchronization mechanism in the function chcr_ktls_dev_add().
>>
>> Consider the following execution scenario:
>>
>>   chcr_ktls_cpl_act_open_rpl()   //641
>>     u_ctx = adap->uld[CXGB4_ULD_KTLS].handle;   //686
>>     if (u_ctx) {  //687
>>     complete(&tx_info->completion);  //704
>>
>> The variable u_ctx is checked by an if statement at Line 687, which means
>> it can be NULL. Then, complete() is called at Line 704, which will wake
>> up wait_for_completion_xxx().
>>
>> Consider the wait_for_completion_timeout() in chcr_ktls_dev_add():
>>
>>   chcr_ktls_dev_add()  //412
>>     u_ctx = adap->uld[CXGB4_ULD_KTLS].handle;  //432
>>     wait_for_completion_timeout(&tx_info->completion, 30 * HZ); //551
>>     xa_erase(&u_ctx->tid_list, tx_info->tid);  //580
>>
>> The variable u_ctx is dereferenced without being rechecked at Line 580
>> after the wait_for_completion_timeout(), which can introduce a null-pointer
>> dereference. Besides, the variable u_ctx is also checked at Line 442 in
>> chcr_ktls_dev_add(), which indicates that u_ctx is likely to be NULL in
>> some execution contexts.
>>
>> To fix this possible null-pointer dereference, a NULL check is put ahead of
>> the call to xa_erase().
>>
>> This potential bug was discovered using an experimental static analysis
>> tool developed by our team. The tool deduces complete() and
>> wait_for_completion() pairs using alias analysis. It then applies data
>> flow analysis to detect null-pointer dereferences across synchronization
>> points.
>>
>> Fixes: 65e302a9bd57 ("cxgb4/ch_ktls: Clear resources when pf4 device is removed")
>> Signed-off-by: Tuo Li <islituo@...il.com>
>
> Hi Tuo Li,
>
> I do see that the checking of u_ctx is inconsistent,
> but it is not clear to me that is because one part is too defensive
> or, OTOH, there is a bug as you suggest. And I think that we need
> more analysis to determine which case it is.
>
> Also, if it is the case that there is a bug as you suggest, after a quick
> search, I think it also exists in at least one other place in this file.
>
> ...