Re: [PATCH net] vsock/virtio: fix skb overhead overflow on 32-bit builds

[David Laight <david.laight.linux@gmail.com>]

On Mon, 25 May 2026 15:09:54 +0200
Stefano Garzarella <sgarzare@redhat.com> wrote:

> On Mon, May 25, 2026 at 08:42:01AM -0400, Michael S. Tsirkin wrote:
> >On Mon, May 25, 2026 at 11:53:14AM +0100, David Laight wrote:  
> >> On Mon, 25 May 2026 11:57:45 +0200
> >> Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>  
> >> > On Sat, May 23, 2026 at 05:35:57PM +0100, David Laight wrote:  
> >> > >On Sat, 23 May 2026 02:20:29 +0000
> >> > >patchwork-bot+netdevbpf@kernel.org wrote:
> >> > >  
> >> > >> Hello:
> >> > >>
> >> > >> This patch was applied to netdev/net.git (main)
> >> > >> by Jakub Kicinski <kuba@kernel.org>:  
> >> > >
> >> > >Did anyone else notice that is isn't a bug?
> >> > >
> >> > >There is no way that a 'count of bytes of kernel memory' can overflow
> >> > >the size of 'long'.  
> >> >
> >> > It's more of an estimate than an actual calculation of memory usage if
> >> > we queue the incoming packet. In theory, an overflow could occur if the
> >> > user sets `buf_alloc` to 4GB. In practice, though, I think you're right:
> >> > the memory should run out before we get to that check.  
> >>
> >> The calculation is:
> >>
> >> 	u64 skb_overhead = (skb_queue_len(&vvs->rx_queue) + 1) * SKB_TRUESIZE(0);
> >>
> >> skb_queue_len() will be the number of items on the queue.
> >> SKB_TRUESIZE(0) is the memory taken up by a zero length skb (basically sizeof(skb)).
> >>
> >> Unless you either corrupt the queue length or manage to allocate skb that use
> >> less than the minimum about of memory that product can't overflow 'unsigned long'.
> >>
> >> The later calculations might wrap - but the multiply can't.
> >>
> >> -- David  
> >
> >
> >Indeed, I wasn't thinking. For this to even get close to overflowing
> >we'd have to have almost all of 4G available to the 32 bit kernel taken
> >up by this single queue.

Except there is usually only 1G or 2G available to the kernel.
And all the skb would have to contain no data.

> >
> >Revert, I'd say.  
> 
> I also blindly added the cast to silence sashiko :-(
> I see now that it could never actually happen, but semantically it’s 
> correct, so maybe we can avoid the revert.

Lots of things are semantically correct :-)

I didn't look any further down the function to see if it could be
'unsigned long' (or even size_t - but I like 'proper' types when they
are always correct, I have to remember that size_t is unsigned long).

The problem with the (u64) cast is that gcc is very likely to make a
'pigs breakfast' of it and do a full 64x64 multiply.
It'll then try to keep the 64bit value in a register-pair which ends
up being spilled to stack as a pair.
I've seen it spill a constant zero and do a multiply by an immediate
zero when doing 64bit maths on 32bit x86.
I think gcc can hold a 64bit value as two separate 32bit values; that
can generate reasonable code. But if they get merged (eg because of an
"=A" asm constraint) it all goes horribly wrong.
This is why there are some asm 'helpers' for mixed 32bit/64bit maths.

-- David

> 
> Thanks,
> Stefano
> 
> >  
> >> >
> >> > Thanks,
> >> > Stefano
> >> >  
> >> > >
> >> > >-- David
> >> > >  
> >> > >>
> >> > >> On Thu, 21 May 2026 14:47:32 +0200 you wrote:  
> >> > >> > From: Stefano Garzarella <sgarzare@redhat.com>
> >> > >> >
> >> > >> > On 32-bit architectures, both skb_queue_len() and SKB_TRUESIZE(0) evaluate
> >> > >> > to 32-bit values. The multiplication can overflow before being assigned to
> >> > >> > the u64 skb_overhead variable, making the skb overhead check ineffective.
> >> > >> >
> >> > >> > Cast skb_queue_len() to u64 so the multiplication is always performed in
> >> > >> > 64-bit arithmetic.
> >> > >> >
> >> > >> > [...]  
> >> > >>
> >> > >> Here is the summary with links:
> >> > >>   - [net] vsock/virtio: fix skb overhead overflow on 32-bit builds
> >> > >>     https://git.kernel.org/netdev/net/c/4157501b9a8f
> >> > >>
> >> > >> You are awesome, thank you!  
> >> > >  
> >> >  
> >  
>