Re: [PATCH] net: fix signed shift overflow in IPv6 phdr cksum

[Thomas Monjalon <thomas@monjalon.net>]

Should we merge this patch and expect another one for the endian issue?


21/09/2026 23:15, Stephen Hemminger:
> On Fri, 18 Sep 2026 10:47:29 +0300
> Aleksandr Khromov <haa@amicon.ru> wrote:
> 
> > In rte_ipv6_phdr_cksum() the next header field, a uint8_t, is promoted to
> > a signed int before the left shift by 24. For protocol values >= 128
> > (for example IPPROTO_SCTP), proto << 24 does not fit in int, which is
> > undefined behaviour (signed left shift overflow) reported by UBSan:
> > 
> >   rte_ip6.h: runtime error: left shift of 132 by 24 places cannot be
> >   represented in type 'int'
> > 
> > Cast the operand to uint32_t before the shift so it is performed in
> > unsigned arithmetic. The resulting value is unchanged on two's
> > complement platforms. The same idiom is already used in RTE_IPV4().
> > 
> > Fixes: 6006818cfb26 ("net: new checksum functions")
> > Cc: stable@dpdk.org
> > Signed-off-by: Aleksandr Khromov <haa@amicon.ru>
> > ---
> 
> Looks good, but there is also a pre-existing byte order issue here.
> 
> Review: [PATCH] net: fix signed shift overflow in IPv6 phdr cksum
> Patchwork: 169805
> 
> Applies cleanly to main. Fixes: 6006818cfb26 verified; the line was
> introduced there and carried over by 1a2b549bb4 (header split).
> Cc: stable is correct.
> 
> The fix is right. ipv6_hdr->proto is uint8_t, promoted to int, and
> proto << 24 for proto >= 128 (SCTP = 132) overflows int. Casting the
> operand to uint32_t makes the shift unsigned; generated code is the
> same.
> 
> Info
> 
>   drivers/net/hinic/hinic_pmd_tx.c:740 has an identical copy of this
>   code with the same UB:
> 
>       psd_hdr.proto = (ipv6_hdr->proto << 24);
> 
>   Worth fixing in the same patch (or a v2 as a two patch series) so
>   the pattern does not survive in the driver.
> 
>   Consider rte_cpu_to_be_32(ipv6_hdr->proto) instead of the shift.
>   psd_hdr.proto is rte_be32_t and must hold proto in the last byte in
>   memory. proto << 24 only achieves that on little-endian; on a
>   big-endian build it lands in the first byte and the pseudo-header
>   sum is wrong. rte_cpu_to_be_32() removes the shift, is correct for
>   both byte orders, and the compiler folds it to the same shift on
>   little-endian. Not a blocker since the endian issue is pre-existing.
> 
>