Re: [PATCH net v3 2/2] sctp: auth: verify auth requirement when auth_chunk is NULL

On Mon, Jul 20, 2026 at 5:32=E2=80=AFAM luoqing <[email protected]> wrote=
:
>
> From: Qing Luo <[email protected]>
>
> sctp_auth_chunk_verify() currently returns true unconditionally
> when chunk->auth_chunk is NULL, which means authentication is
> silently skipped. This is incorrect in two scenarios:
>
> 1. skb_clone() failed in the BH receive path, leaving auth_chunk
>    NULL. Although the previous fix avoids setting auth=3D1 in this
>    case, the chunk can still reach sctp_auth_chunk_verify() via
>    sctp_endpoint_bh_rcv() where asoc is NULL for new connections,
>    bypassing the early sctp_auth_recv_cid() check.
>
> 2. No AUTH chunk precedes COOKIE-ECHO in the packet. In this case
>    skb_clone() is never called and auth_chunk remains NULL. Again,
>    in sctp_endpoint_bh_rcv() the early check cannot catch this
>    because asoc is NULL and sctp_auth_recv_cid() returns 0.
>
> Fix by checking sctp_auth_recv_cid() when auth_chunk is NULL:
> if authentication is required for this chunk type, return false
> to drop the chunk; otherwise, continue normally.
>
> Fixes: bbd0d59809f9 ("[SCTP]: Implement the receive and verification of A=
UTH chunk")
> Signed-off-by: Qing Luo <[email protected]>
> ---
>  net/sctp/sm_statefuns.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index d23d935e128e..89ed618b1de3 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -642,7 +642,7 @@ static bool sctp_auth_chunk_verify(struct net *net, s=
truct sctp_chunk *chunk,
>         struct sctp_chunk auth;
>
>         if (!chunk->auth_chunk)
> -               return true;
> +               return !sctp_auth_recv_cid(chunk->chunk_hdr->type, asoc);
>
>         /* SCTP-AUTH:  auth_chunk pointer is only set when the cookie-ech=
o
>          * is supposed to be authenticated and we have to do delayed
> --
> 2.25.1
> >> A better fix would be:
> >>
> >> Add a check in sctp_auth_chunk_verify() at the point where the COOKIE-=
ECHO
> >> chunk is actually being processed:
> >>
> >>
> >>         if (!chunk->auth_chunk)
> >>                 return !sctp_auth_recv_cid(chunk->chunk_hdr->type, aso=
c);
> >>
> >> This ensures that if chunk->auth_chunk is missing while authentication=
 is
> >> required for the COOKIE-ECHO chunk, the verification fails and the chu=
nk is
> >> dropped. Otherwise, when authentication is not required, processing ca=
n
> >> continue normally.
> >>
> >> Please give it a try.
> >>
> > Also, please add a extra Fixes tag in your next post:
> >
> > Fixes: bbd0d59809f9 ("[SCTP]: Implement the receive and verification
> > of AUTH chunk")
> >
> > which introduces chunk->auth_chunk and calls skb_clone() in
> > sctp_endpoint_bh_rcv().
> Hi,
>
> Thanks for the review. I=E2=80=99ve reworked the fix into two patches:
>
> Patch 1/2: In sctp_assoc_bh_rcv() and sctp_endpoint_bh_rcv(), only set ch=
unk->auth =3D 1 when skb_clone() succeeds.
>
> Patch 2/2: In sctp_auth_chunk_verify(), when auth_chunk is NULL, check sc=
tp_auth_recv_cid() to decide whether authentication is required. This cover=
s both cases from the review.
>
> I=E2=80=99d like to discuss whether Patch 1 is necessary. Patch 2 alone i=
s sufficient for correctness =E2=80=94 even with auth =3D=3D 1 and auth_chu=
nk =3D=3D NULL, Patch 2 catches it at the verification point. Patch 1 only =
provides semantic cleanliness (not setting auth =3D 1 without a valid auth_=
chunk), but closes no additional gap.
>
> Should I keep Patch 1 as a defensive cleanup, or drop it and submit only =
Patch 2?
>
I agree that adding the chunk->auth_chunk check makes the logic clearer.

However, the intention behind skipping chunk->auth =3D 1 is to drop the
packet earlier. During the normal handshake path (sctp_endpoint_bh_rcv()),
asoc is NULL, and sctp_auth_recv_cid() always returns 0. As a result, not
setting chunk->auth =3D 1 does not actually achieve the intended effect in
this case.

Given that, I think it would be better to simply break the loop in both
functions:

if (!chunk->auth_chunk)
        break;
chunk->auth =3D 1;

Can you move forward with the 2/2 patch only for this issue? and post the
1/2 patch to 'net-next' as an improvement.

Thanks.