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

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

Date:   Sat, 28 Jan 2023 17:57:06 +0100
From:   Pietro Borrello <borrello@...g.uniroma1.it>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     Boris Pismenny <borisp@...dia.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Vakul Garg <vakul.garg@....com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>, Jakob Koschel <jkl820.git@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry

On Sat, 28 Jan 2023 at 17:40, Simon Horman <simon.horman@...igine.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