Date:   Fri, 10 Feb 2023 19:30:39 -0800
From:   Jakub Kicinski <kuba@kernel.org>
To:     Pietro Borrello <borrello@diag.uniroma1.it>
Cc:     Neil Horman <nhorman@tuxdriver.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
        Xin Long <lucien.xin@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>, Jakob Koschel <jkl820.git@gmail.com>,
        linux-sctp@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2] sctp: sctp_sock_filter(): avoid
 list_entry() on possibly empty list
Message-ID: <20230210193039.12accfa5@kernel.org>
In-Reply-To: <20230208-sctp-filter-v2-1-6e1f4017f326@diag.uniroma1.it>
References: <20230208-sctp-filter-v2-1-6e1f4017f326@diag.uniroma1.it>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 09 Feb 2023 12:13:05 +0000 Pietro Borrello wrote:
> The list_entry on an empty list creates a type confused pointer.
> While using it is undefined behavior, in this case it seems there
> is no big risk, as the `tsp->asoc != assoc` check will almost
> certainly fail on the type confused pointer.
> We report this bug also since it may hide further problems since
> the code seems to assume a non-empty `ep->asocs`.
> 
> We were able to trigger sctp_sock_filter() using syzkaller, and
> cause a panic inserting `BUG_ON(list_empty(&ep->asocs))`, so the
> list may actually be empty.
> But we were not able to minimize our testcase and understand how
> sctp_sock_filter may end up with an empty asocs list.
> We suspect a race condition between a connecting sctp socket
> and the diag query.
> 
> We attach the stacktrace when triggering the injected
> `BUG_ON(list_empty(&ep->asocs))`:

Thanks for the analysis, but I'll put this in for 6.2 anyway.
The patch looks fairly straightforward / provably correct, and
with the Fixes tag present chances are it will end up in stable either
way. With a difference of maybe a week, since the merge window is just
a week away..