Re: [PATCH net 1/4] xsk: cache csum_start/csum_offset to fix TOCTOU in xsk_skb_metadata()

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 05/13, Jason Xing wrote:
> On Wed, May 13, 2026 at 6:34 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > On 05/12, Jason Xing wrote:
> > > On Mon, May 11, 2026 at 11:03 PM Stanislav Fomichev
> > > <sdf.kernel@gmail.com> wrote:
> > > >
> > > > On 05/10, Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@tencent.com>
> > > > >
> > > > > The TX metadata area resides in the UMEM buffer which is memory-mapped
> > > > > and concurrently writable by userspace. In xsk_skb_metadata(),
> > > > > csum_start and csum_offset are read from shared memory for bounds
> > > > > validation, then read again for skb assignment. A malicious userspace
> > > > > application can race to overwrite these values between the two reads,
> > > > > bypassing the bounds check and causing out-of-bounds memory access
> > > > > during checksum computation in the transmit path.
> > > > >
> > > > > Fix this by reading csum_start and csum_offset into local variables
> > > > > once, then using the local copies for both validation and assignment.
> > > > >
> > > > > Note that other metadata fields (flags, launch_time) and the cached
> > > > > csum fields may be mutually inconsistent due to concurrent userspace
> > > > > writes, but this is benign: the only security-critical invariant is
> > > > > that each field's validated value is the same one used, which local
> > > > > caching guarantees.
> > > > >
> > > > > Closes: https://lore.kernel.org/all/20260503200927.73EA1C2BCB4@smtp.kernel.org/
> > > > > Fixes: 48eb03dd2630 ("xsk: Add TX timestamp and TX checksum offload support")
> > > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > > ---
> > > > >  net/xdp/xsk.c | 11 +++++++----
> > > > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > > index 6bcd77068e52..cd039e397018 100644
> > > > > --- a/net/xdp/xsk.c
> > > > > +++ b/net/xdp/xsk.c
> > > > > @@ -722,6 +722,7 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> > > > >                           u32 hr)
> > > > >  {
> > > > >       struct xsk_tx_metadata *meta = NULL;
> > > > > +     u16 csum_start, csum_offset;
> > > > >
> > > > >       if (unlikely(pool->tx_metadata_len == 0))
> > > > >               return -EINVAL;
> > > > > @@ -731,13 +732,15 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> > > > >               return -EINVAL;
> > > > >
> > > > >       if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
> > > > > -             if (unlikely(meta->request.csum_start +
> > > > > -                          meta->request.csum_offset +
> > > > > +             csum_start = meta->request.csum_start;
> > > > > +             csum_offset = meta->request.csum_offset;
> > > >
> > > > Wondering if it's better to READ_ONCE(x) these?
> > >
> > > I still chose not to use it after reading the suggestion from local
> > > AI. The reason is there is no WRITE_ONCE pair to make sure everything
> > > is no data-race. I also checked some existing implementations around
> > > the shared buffer (between userspace and kernel) and didn't manage to
> > > see the usage of XXXX_ONCE(). Does it make any sense to you :) ?
> >
> > Without READ_ONCE your patch relies on the compiler honoring exactly the
> > loads and stores as written. Which I don't think it does (hence that
> > whole WRITE_ONCE/READ_ONCE mess). IOW, it can pretty much generate
> > the same code (and read csum_start twice) even with your patch applied. Happy
> > to argue with your local AI if you give me the output :-D
> >
> > I grepped io_uring/ and I see similar pattern for reading user supplied
> > entries (via READ_ONCE).
> 
> I roughly understand what your meaning is here. My thought is the
> data-race condition in this case still happens even with the
> READ_ONCE() protection (because no such corresponding operation is
> performed on the writer side).
> 
> Actually the local AI said use READ_ONCE instead.
> 
> I can change this as you suggested for sure. Maybe related fields can
> be protected in the same way in another patch.

In general, feels like we should be doing READ_ONCE on all user-supplied
descriptors (to make it more apparent that those can be concurrently changed
by the userspace). In practice, probably too late and too much churn to update
everything at this point, so let's only fix the ones that can lead to
TOCTOU.