On Wed, Apr 6, 2022 at 4:21 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > On Wed, Apr 6, 2022 at 9:34 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > On Tue, Apr 5, 2022 at 1:58 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > On Mon, Apr 4, 2022 at 6:15 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > > > > > Adding LSM and SELinux lists to CC for awareness; the original patch > > > > is available at: > > > > https://lore.kernel.org/netdev/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@xxxxxxxxx/T/ > > > > https://patchwork.kernel.org/project/netdevbpf/patch/a77a584b3ce9761eb5dda5828192e1cab94571f0.1649037151.git.lucien.xin@xxxxxxxxx/ > > > > > > > > On Mon, Apr 4, 2022 at 3:53 AM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > > > > > > > > Yi Chen reported an unexpected sctp connection abort, and it occurred when > > > > > COOKIE_ECHO is bundled with DATA Fragment by SCTP HW GSO. As the IP header > > > > > is included in chunk->head_skb instead of chunk->skb, it failed to check > > > > > IP header version in security_sctp_assoc_request(). > > > > > > > > > > According to Ondrej, SELinux only looks at IP header (address and IPsec > > > > > options) and XFRM state data, and these are all included in head_skb for > > > > > SCTP HW GSO packets. So fix it by using head_skb when calling > > > > > security_sctp_assoc_request() in processing COOKIE_ECHO. > > > > > > > > The logic looks good to me, but I still have one unanswered concern. > > > > The head_skb member of struct sctp_chunk is defined inside a union: > > > > > > > > struct sctp_chunk { > > > > [...] > > > > union { > > > > /* In case of GSO packets, this will store the head one */ > > > > struct sk_buff *head_skb; > > > > /* In case of auth enabled, this will point to the shkey */ > > > > struct sctp_shared_key *shkey; > > > > }; > > > > [...] > > > > }; > > > > > > > > What guarantees that this chunk doesn't have "auth enabled" and the > > > > head_skb pointer isn't actually a non-NULL shkey pointer? Maybe it's > > > > obvious to a Linux SCTP expert, but at least for me as an outsider it > > > > isn't - that's usually a good hint that there should be a code comment > > > > explaining it. > > > Hi Ondrej, > > > > > > shkey is for tx skbs only, while head_skb is for skbs on rx path. > > > > That makes sense, thanks. I would still be happier if this was > > documented, but the comment would best fit in the struct sctp_chunk > > definition and that wouldn't fit in this patch... > > > > Actually I have one more question - what about the > > security_sctp_assoc_established() call in sctp_sf_do_5_1E_ca()? Is > > COOKIE ACK guaranteed to be never bundled? > COOKIE ACK could also be bundled with DATA. > I didn't change it as it would not break SCTP. > (security_inet_conn_established() returns void) > But I don't mind changing it if you think it's necessary. security_inet_conn_established? Are you looking at an old version of the code, perhaps? In mainline, sctp_sf_do_5_1E_ca() now calls the new security_sctp_assoc_established() hook, which may return an error. But even if it didn't, I believe we want to make sure that an skb with valid inet headers and XFRM state is passed to the hooks as SELinux relies on these to correctly process the SCTP association. -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
