Re: [PATCH net v3] sctp: auth: propagate HMAC calculation errors to callers

On Tue, Aug 4, 2026 at 4:59 AM luoqing <[email protected]> wrote:
>
> From: Qing Luo <[email protected]>
>
> sctp_auth_calculate_hmac() can fail when building the association secret
> under memory pressure, but its void return silently leaves the HMAC digest
> zeroed.  On the receive path, sctp_sf_authenticate() compares this zeroed
> digest against the peer-supplied one using crypto_memneq(), potentially
> accepting an all-zero HMAC from the peer if the allocation failed.  A peer
> can reach this path with a configured but non-active shared key id, so
> association setup is affected too.  Although the allocation failure itself
> is not attacker controlled, the incorrect acceptance is a security issue.
>
> Fix this by making sctp_auth_calculate_hmac() return int:
> - sctp_sf_authenticate() returns SCTP_IERROR_NOMEM instead of accepting
>   a zero HMAC.
> - sctp_auth_chunk_verify() propagates the ierror so delayed COOKIE ECHO
>   and other paths see NOMEM vs BAD_SIG correctly.
> - sctp_packet_pack() drops the packet on failure instead of transmitting
>   a zeroed HMAC that the peer would reject.
>
> Update the declaration in auth.h accordingly.
>
> Fixes: 1f485649f529 ("[SCTP]: Implement SCTP-AUTH internals")
> Cc: [email protected]
> Assisted-by: LLM:code-review
> Signed-off-by: Qing Luo <[email protected]>
> ---
Please add a revision changelog here describing the changes since the
previous version, similar to:

https://lore.kernel.org/netdev/[email protected]/


>  include/net/sctp/auth.h |  6 ++---
>  net/sctp/auth.c         | 10 ++++----
>  net/sctp/output.c       | 12 +++++++---
>  net/sctp/sm_statefuns.c | 51 ++++++++++++++++++++++++++++++-----------
>  4 files changed, 56 insertions(+), 23 deletions(-)
>
> diff --git a/include/net/sctp/auth.h b/include/net/sctp/auth.h
> index 6f2cd562b1de..eeb3297fe97d 100644
> --- a/include/net/sctp/auth.h
> +++ b/include/net/sctp/auth.h
> @@ -83,9 +83,9 @@ int sctp_auth_send_cid(enum sctp_cid chunk,
>                        const struct sctp_association *asoc);
>  int sctp_auth_recv_cid(enum sctp_cid chunk,
>                        const struct sctp_association *asoc);
> -void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> -                             struct sk_buff *skb, struct sctp_auth_chunk *auth,
> -                             struct sctp_shared_key *ep_key, gfp_t gfp);
> +int sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> +                            struct sk_buff *skb, struct sctp_auth_chunk *auth,
> +                            struct sctp_shared_key *ep_key, gfp_t gfp);
>  void sctp_auth_shkey_release(struct sctp_shared_key *sh_key);
>  void sctp_auth_shkey_hold(struct sctp_shared_key *sh_key);
>
> diff --git a/net/sctp/auth.c b/net/sctp/auth.c
> index c901d373af80..6de66f56c41c 100644
> --- a/net/sctp/auth.c
> +++ b/net/sctp/auth.c
> @@ -613,9 +613,9 @@ int sctp_auth_recv_cid(enum sctp_cid chunk, const struct sctp_association *asoc)
>   *    zero (as shown in Figure 6) followed by all chunks that are placed
>   *    after the AUTH chunk in the SCTP packet.
>   */
> -void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> -                             struct sk_buff *skb, struct sctp_auth_chunk *auth,
> -                             struct sctp_shared_key *ep_key, gfp_t gfp)
> +int sctp_auth_calculate_hmac(const struct sctp_association *asoc,
> +                            struct sk_buff *skb, struct sctp_auth_chunk *auth,
> +                            struct sctp_shared_key *ep_key, gfp_t gfp)
>  {
>         struct sctp_auth_bytes *asoc_key;
>         __u16 key_id, hmac_id;
> @@ -636,7 +636,7 @@ void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
>                 /* ep_key can't be NULL here */
>                 asoc_key = sctp_auth_asoc_create_secret(asoc, ep_key, gfp);
>                 if (!asoc_key)
> -                       return;
> +                       return -ENOMEM;
>
>                 free_key = 1;
>         }
> @@ -654,6 +654,8 @@ void sctp_auth_calculate_hmac(const struct sctp_association *asoc,
>
>         if (free_key)
>                 sctp_auth_key_put(asoc_key);
> +
> +       return 0;
>  }
>
>  /* API Helpers */
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 23e96305cad7..3d7ead9d40e1 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -517,8 +517,14 @@ static int sctp_packet_pack(struct sctp_packet *packet,
>                 }
>
>                 if (auth) {
> -                       sctp_auth_calculate_hmac(tp->asoc, nskb, auth,
> -                                                packet->auth->shkey, gfp);
> +                       if (sctp_auth_calculate_hmac(tp->asoc, nskb, auth,
> +                                                    packet->auth->shkey, gfp)) {
> +                               sctp_chunk_free(packet->auth);
> +                               packet->auth = NULL;
> +                               if (gso)
> +                                       kfree_skb(nskb);
> +                               return -ENOMEM;
Returning 0 here is fine, as its only caller, sctp_packet_transmit(),
currently always returns 0. sctp_packet_pack(), on the other hand, returns
the number of packets it builds.

If you'd like to improve the return value for sctp_packet_transmit(),
that can be done in a separate patch targeting net-next.

> +                       }
>                         /* free auth if no more chunks, or add it back */
>                         if (list_empty(&packet->chunk_list))
>                                 sctp_chunk_free(packet->auth);
> @@ -619,7 +625,7 @@ int sctp_packet_transmit(struct sctp_packet *packet, gfp_t gfp)
>
>         /* pack up chunks */
>         pkt_count = sctp_packet_pack(packet, head, gso, gfp);
> -       if (!pkt_count) {
> +       if (pkt_count <= 0) {
>                 kfree_skb(head);
>                 goto out;
>         }
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 708fa07d5fff..e19881c90b49 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -637,13 +637,17 @@ enum sctp_disposition sctp_sf_do_5_1C_ack(struct net *net,
>         return SCTP_DISPOSITION_CONSUME;
>  }
>
> -static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
> -                                  const struct sctp_association *asoc)
> +static enum sctp_ierror sctp_auth_chunk_verify(struct net *net,
> +                                              struct sctp_chunk *chunk,
> +                                              const struct sctp_association *asoc)
>  {
>         struct sctp_chunk auth;
>
> -       if (!chunk->auth_chunk)
> -               return !sctp_auth_recv_cid(chunk->chunk_hdr->type, asoc);
> +       if (!chunk->auth_chunk) {
> +               if (sctp_auth_recv_cid(chunk->chunk_hdr->type, asoc))
> +                       return SCTP_IERROR_BAD_SIG;
> +               return SCTP_IERROR_NO_ERROR;
> +       }
>
>         /* SCTP-AUTH:  auth_chunk pointer is only set when the cookie-echo
>          * is supposed to be authenticated and we have to do delayed
> @@ -654,7 +658,7 @@ static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
>
>         /* Make sure that we and the peer are AUTH capable */
>         if (!net->sctp.auth_enable || !asoc->peer.auth_capable)
> -               return false;
> +               return SCTP_IERROR_BAD_SIG;
>
>         /* set-up our fake chunk so that we can process it */
>         auth.skb = chunk->auth_chunk;
> @@ -666,7 +670,7 @@ static bool sctp_auth_chunk_verify(struct net *net, struct sctp_chunk *chunk,
>         skb_pull(chunk->auth_chunk, sizeof(struct sctp_chunkhdr));
>         auth.transport = chunk->transport;
>
> -       return sctp_sf_authenticate(asoc, &auth) == SCTP_IERROR_NO_ERROR;
> +       return sctp_sf_authenticate(asoc, &auth);
>  }
>
Please drop the change to sctp_auth_chunk_verify(). This patch is intended
as a fix, and code improvements should be submitted separately to net-next.

Thanks.