Re: [PATCH net-next v4] sctp: auth: propagate HMAC calculation errors to callers

On Fri, Aug 7, 2026 at 2:44 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.  On the
> send path, sctp_packet_pack() transmits a packet with a zeroed HMAC that
> the peer would reject.
>
> Improve error handling by making sctp_auth_calculate_hmac() return int:
> - sctp_sf_authenticate() returns SCTP_IERROR_NOMEM instead of accepting
>   a zero HMAC.
> - sctp_packet_pack() drops the packet on failure instead of transmitting
>   a zeroed HMAC.
>
> Update the declaration in auth.h accordingly.
>
> Assisted-by: LLM
> Signed-off-by: Qing Luo <[email protected]>

Fixes: 1f485649f529 ("[SCTP]: Implement SCTP-AUTH internals")
Acked-by: Xin Long <[email protected]>

>
> Hi Longxin
>
> Thank you very much for your patient guidance on community patch submission conventions and your valuable review. I fully take your comment.
>
> As you mentioned, returning 0 is acceptable for the moment, given `sctp_packet_transmit()` always returns zero today, while `sctp_packet_pack()` returns the count of generated packets.
>
> Although there is no functional bug in the current code, unifying the return‑value semantics makes the interface more consistent, which can prevent latent risks when this code path is extended in the future.
>
> I will implement this optimization as a standalone patch for net‑next as you suggested.
>
> Please help to evaluate whether this improvement is necessary for mainline. If maintainers think it brings limited practical benefit, I am okay to drop this patch entirely.
>

It's fine to make the code look cleaner even if there's little practical
benefit. However, for the sctp_auth_chunk_verify() change, I don't think
it looks better for all its callers to have:

+       error = sctp_auth_chunk_verify(net, chunk, new_asoc);
+       if (error != SCTP_IERROR_NO_ERROR) {
+               if (error == SCTP_IERROR_NOMEM)
+                       return SCTP_DISPOSITION_NOMEM;
                return SCTP_DISPOSITION_DISCARD;
+       }

If there's no clean way to improve it, I'd prefer to keep
sctp_auth_chunk_verify() returning bool.

About the return value of sctp_packet_transmit(), I just checked the
history and found this comment in the code:

  /* FIXME: Returning the 'err' will effect all the associations
   * associated with a socket, although only one of the paths of the
   * association is unreachable.
   * The real failure of a transport or association can be passed on
   * to the user via notifications. So setting this error may not be
   * required.
   */
   /* err = -EHOSTUNREACH; */

I think that's why its return value is currently always 0. IMHO,
sctp_packet_transmit() should return void, and all its callers should stop
processing the return value.

Thanks.