[PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

[PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

[Pietro Borrello]

tls_is_tx_ready() checks that list_first_entry() does not return NULL.
This condition can never happen. For empty lists, list_first_entry()
returns the list_entry() of the head, which is a type confusion.
Use list_first_entry_or_null() which returns NULL in case of empty
lists.

Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
---
 net/tls/tls_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 9ed978634125..a83d2b4275fa 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2427,7 +2427,7 @@ static bool tls_is_tx_ready(struct tls_sw_context_tx *ctx)
 {
 	struct tls_rec *rec;
 
-	rec = list_first_entry(&ctx->tx_list, struct tls_rec, list);
+	rec = list_first_entry_or_null(&ctx->tx_list, struct tls_rec, list);
 	if (!rec)
 		return false;
 

---
base-commit: 2241ab53cbb5cdb08a6b2d4688feb13971058f65
change-id: 20230128-list-entry-null-check-tls-92da3440d32e

Best regards,
-- 
Pietro Borrello <borrello@diag.uniroma1.it>

Re: [PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

[Simon Horman]

On Sat, Jan 28, 2023 at 04:29:17PM +0000, Pietro Borrello wrote:
> tls_is_tx_ready() checks that list_first_entry() does not return NULL.
> This condition can never happen. For empty lists, list_first_entry()
> returns the list_entry() of the head, which is a type confusion.
> Use list_first_entry_or_null() which returns NULL in case of empty
> lists.
> 
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
>  net/tls/tls_sw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 9ed978634125..a83d2b4275fa 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2427,7 +2427,7 @@ static bool tls_is_tx_ready(struct tls_sw_context_tx *ctx)
>  {
>  	struct tls_rec *rec;
>  
> -	rec = list_first_entry(&ctx->tx_list, struct tls_rec, list);
> +	rec = list_first_entry_or_null(&ctx->tx_list, struct tls_rec, list);
>  	if (!rec)
>  		return false;

Hi Pietro,

I agree this is correct.

However, given that the code has been around for a while,
I feel it's relevant to ask if tx_list can ever be NULL.
If not, perhaps it's better to remove the error path entirely.

Re: [PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

[Pietro Borrello]

On Sat, 28 Jan 2023 at 17:40, Simon Horman <simon.horman@corigine.com> wrote:
>
> Hi Pietro,
>
> I agree this is correct.
>
> However, given that the code has been around for a while,
> I feel it's relevant to ask if tx_list can ever be NULL.
> If not, perhaps it's better to remove the error path entirely.

Hi Simon,
Thank you for your fast reply.
The point is exactly that tx_list will never be NULL, as the list head will
always be present.
So the error, as is, will never be detected, resulting in type confusion.
We found this with static analysis, so we have no way to say for sure that
the list can never be empty on edge cases.
As this is a type confusion, the errors are often sneaky and go undetected.

As an example, the following bug we previously reported resulted in a type
confusion on net code that went undetected for more than 20 years.
Link: https://lore.kernel.org/all/9fcd182f1099f86c6661f3717f63712ddd1c676c.1674496737.git.marcelo.leitner@gmail.com/
In that case, we were able to create a PoC to demonstrate the issue where we
leveraged the type confusion to bypass KASLR.

In the end, this is the maintainer's call, but I would keep the check and
correctly issue a list_first_entry_or_null() so that the check will work
as intended as the added overhead is just a pointer comparison which
would likely justify the cost of a more solid code.
Otherwise, I can also submit a patch that entirely removes the check.
Let me know what you prefer.

Best regards,
Pietro

Re: [PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Sat, 28 Jan 2023 16:29:17 +0000 you wrote:
> tls_is_tx_ready() checks that list_first_entry() does not return NULL.
> This condition can never happen. For empty lists, list_first_entry()
> returns the list_entry() of the head, which is a type confusion.
> Use list_first_entry_or_null() which returns NULL in case of empty
> lists.
> 
> Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> 
> [...]

Here is the summary with links:
  - [net-next] net/tls: tls_is_tx_ready() checked list_entry
    https://git.kernel.org/netdev/net/c/ffe2a2256244

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html