MIME-Version: 1.0
References: <20230128-list-entry-null-check-tls-v1-1-525bbfe6f0d0@diag.uniroma1.it>
 <Y9VP6Hw7jH0VelUX@corigine.com>
In-Reply-To: <Y9VP6Hw7jH0VelUX@corigine.com>
From:   Pietro Borrello <borrello@diag.uniroma1.it>
Date:   Sat, 28 Jan 2023 17:57:06 +0100
Message-ID: <CAEih1qX-XG=-OxMcNyWm9NuYG+_=oFHkTPD3s-Q7EPKPAS3+zw@mail.gmail.com>
Subject: Re: [PATCH net-next] net/tls: tls_is_tx_ready() checked list_entry
To:     Simon Horman <simon.horman@corigine.com>
Cc:     Boris Pismenny <borisp@nvidia.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Vakul Garg <vakul.garg@nxp.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>, Jakob Koschel <jkl820.git@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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