Re: [PATCH] 9p/trans_virtio: bound RERROR copy by mapped pages

[Dominique Martinet <asmadeus@codewreck.org>]

Christian Schoenebeck wrote on Mon, Sep 14, 2026 at 06:01:04AM +0200:
> And `len` field is just shared DMA memory written by server. The len value is 
> not questioned anywhere in the call stack, it is taken as-is.
> 
> This is for "split" ring type which QEMU's 9p virtio device is always using 
> (as QEMU's virtio device is not advertising "packed" ring type). Linux 9p 
> client is agnostic about the two, but it does not matter either way, because 
> even with "packed" ring type the call stack would be slightly different, the 
> len field would be 32-bit instead of 16-bit, but its value was still not 
> questioned anywhere.

Thank you for the legwork!

> > If we can't trust req->rc.size I believe the p9pdu processing will also
> > read past the end of buffer so there are much bigger "problems"
> > 
> > With that said, I agree with your assessment that qemu is mostly
> > trusted, and don't want to spend too much effort on this (unless
> > something like me losing my job happens and I suddently find a lot of
> > free time :P), so if you (or someone) can confirm rc.size isn't trusted
> > then let's drop this here.
> 
> Surprisingly though, it is not as bad as one might think first.
> [...]
> Or:
> You could simply pass the bad card on to the virtio subsystem maintainers, and 
> tell them to clamp used-ring element's `len` field against the guest provided 
> descriptor length, then req->rc.size would be trusted - no validation to be 
> done on 9p side. But even then I can imagine that this would be implemented by 
> virtio maintainers as an opt-in solution for drivers like "please clamp this 
> for me" to avoid breaking certain legacy devices.

Given how surprised I found this I could imagine other virtio users to
incorrectly assume the same, so I think it's worth bringing up even if
it doesn't end up implemented (we could actually probably cap this on
the 9p side if we wanted to anyway)

I think truncating rc.size is worth the effort even if most of the paths
are correct, and more appropriate than this current patch, so let's drop
this patch - thank you nevertheless Yizhou Zhao for starting the
discussion.
I'm out of steam though so will take some time to send the
virtualization list a mail.. :)


> > FWIW I said "mostly trusted" because lately we've had things like black
> > box hypervisors (I don't remember the exact name) where guest VMs memory
> > is encrypted and can't be accessed even by the host: sure the server
> > could still *crash* a guest, but it shouldn't be able to get arbitrary
> > write/read primitives, so if someone wants to spend effort fixing such
> > bugs I think it's welcome and I'll be happy to take patches.
> 
> "Confidential Guest Support" is the term. Wouldn't change the picture here 
> though, as this is about a host triggered DoS. Host could still kill the guest 
> at any time.

Hm, yeah, I was confused and considered this as a oob write but it's the
"reading a page that doesn't exist" that fails, so it's not likely to be
too bad even if it does happen.

> The negative dentries patches come to my mind which you might have forgotten; 
> I think v7 is the latest one on these:
> 
> https://lore.kernel.org/all/cover.1779355927.git.repk@triplefau.lt/

I'm not sure why I didn't reply to that thread back then, but it's been
picked up in 7.2 :)

Cheers,
-- 
Dominique Martinet | Asmadeus