Re: [PATCH net-next v10 2/4] net: xdp: add xdp_buff_shrink_tail() helper

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 4506 bytes --]

On Nov 13, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] net: xdp: bounds check split in bpf_xdp_adjust_tail() allows
>   pointer wrap
> --
> 
> commit fe8a696967ad3281fe5305e0444a8948ebb0b12e
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> net: xdp: add xdp_buff_shrink_tail() helper
> 
> This commit moves the XDP tail-shrinking helpers from net/core/filter.c to
> net/core/xdp.c and exports xdp_buff_shrink_tail() for driver use.
> 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 9b8d5191c4ba2..5ea45dfce7a22 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -4438,96 +4438,21 @@ static int bpf_xdp_frags_increase_tail(struct xdp_buff *xdp, int offset)
> 
> [ ... ]
> 
> >  BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
> >  {
> >  	void *data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
> >  	void *data_end = xdp->data_end + offset;
> >  
> > -	if (unlikely(xdp_buff_has_frags(xdp))) { /* non-linear xdp buff */
> > -		if (offset < 0)
> > -			return bpf_xdp_frags_shrink_tail(xdp, -offset);
> > +	if (offset < 0)
> > +		return xdp_buff_shrink_tail(xdp, -offset);
> >  
> > +	if (unlikely(xdp_buff_has_frags(xdp))) /* non-linear xdp buff */
> >  		return bpf_xdp_frags_increase_tail(xdp, offset);
> > -	}
> >  
> >  	/* Notice that xdp_data_hard_end have reserved some tailroom */
> >  	if (unlikely(data_end > data_hard_end))
> >  		return -EINVAL;
> >  
> > -	if (unlikely(data_end < xdp->data + ETH_HLEN))
> > -		return -EINVAL;
> > -
> >  	/* Clear memory area on grow, can contain uninit kernel memory */
> >  	if (offset > 0)
> >  		memset(xdp->data_end, 0, offset);
> 
> [ ... ]
> 
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 470bf90d93ef2..7c98dbc2a770c 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -672,6 +672,97 @@ struct sk_buff *__xdp_build_skb_from_buff(const struct xdp_buff *xdp)
> 
> [ ... ]
> 
> > +int xdp_buff_shrink_tail(struct xdp_buff *xdp, int offset)
> > +{
> > +	void *data_end = xdp->data_end - offset;
> > +
> > +	if (unlikely(offset < 0))
> > +		return -EINVAL;
> > +
> > +	if (unlikely(xdp_buff_has_frags(xdp))) /* non-linear xdp buff */
> > +		return xdp_frags_shrink_tail(xdp, offset);
> > +
> > +	if (unlikely(data_end < xdp->data + ETH_HLEN))
> > +		return -EINVAL;
> > +
> > +	xdp->data_end = data_end;
> > +
> > +	return 0;
> > +}
> 
> [Severity: High]
> Can splitting the upper and lower bounds checks between bpf_xdp_adjust_tail()
> and xdp_buff_shrink_tail() allow pointer wraparound on linear buffers?
> 
> Previously, bpf_xdp_adjust_tail() checked both data_end > data_hard_end and
> data_end < xdp->data + ETH_HLEN on xdp->data_end + offset, which prevented
> pointer arithmetic overflow or underflow from passing validation.
> 
> With the checks split:
> 
> 1. In bpf_xdp_adjust_tail(), offset >= 0 now only checks
>    data_end > data_hard_end. On 32-bit architectures, a large positive offset
>    (e.g. 0x40000000) wraps xdp->data_end + offset past 0 to an address below
>    data_hard_end, bypassing the check and passing offset to
>    memset(xdp->data_end, 0, offset) for an out-of-bounds kernel memory write.
>    It also allows frames initially shorter than ETH_HLEN (such as L3 packets
>    on tun devices in generic XDP) to remain shorter than ETH_HLEN when
>    offset >= 0.
> 
> 2. In xdp_buff_shrink_tail(), linear buffers only check
>    data_end < xdp->data + ETH_HLEN on xdp->data_end - offset. When
>    (uintptr_t)xdp->data_end < 0x80000000 (for example on s390x where
>    PAGE_OFFSET is 0, or 32-bit kernels with VMSPLIT_1G / VMSPLIT_2G_OPT), a
>    positive offset larger than xdp->data_end underflows data_end to a high
>    address above xdp->data + ETH_HLEN, setting xdp->data_end past
>    data_hard_end and exposing out-of-bounds memory to BPF packet accesses.
> 
> Checking offset against the integer buffer bounds (such as
> offset > (int)(xdp->data_end - xdp->data) - ETH_HLEN in
> xdp_buff_shrink_tail(), and keeping the ETH_HLEN and tailroom checks in
> bpf_xdp_adjust_tail() overflow-safe) would avoid both wraparounds.

ack, I will fix it in v11.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261112-stmmac-rx-mb-v10-0-d3d04b65c6bc@oss.qualcomm.com?part=2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]