Re: [PATCH] USB: disable all RNDIS protocol drivers

[Johannes Berg]

On Thu, 2023-07-13 at 07:34 +0200, Greg Kroah-Hartman wrote:
> I wasn't trying to be glib here, sorry if it came across that way.  I'll
> blame the heat...

No worries.

> > All we said is that your statement of "RNDIS is fundamentally unfixable"
> > doesn't make a lot of sense. If this were the case, all USB drivers
> > would have to "trust the other side" as well, right?
> 
> No, well, yes.  See the zillion patches we have had to apply to the
> kernel over the years when someone decided that "usb devices are not to
> be trusted" that syzbot has helped find :)

Sure, I'm well aware of that. But that's also exactly my point - nowhere
has anyone previously suggested that the protocol for any of those
devices is fundamentally broken and the drivers should be removed. We've
fixed those things and moved on.

I can even understand the initial reaction of "oh hey this ancient thing
is probably not used any more, let's just remove it", but even that's a
different reasoning, along the lines of "this has bugs and nobody needs
it". Though that nobody uses it has in fact been proven wrong, which is
pretty much why we're have this discussion at all.

> It's not a DMA issue here, it's a "the protocol allows for buffer
> overflows and does not seem to be able to be verified to prevent this"
> from what I remember (it's been a year since I looked at this last,
> details are hazy.)

If you s/be able to be verified/be verified in the code/ I entirely
believe it, in fact I think it's quite likely given the age of the code
and all. It's just that not being _able_ to verify it seems questionable
to me (and you haven't given any reasons), given that it's USB and you
always have a full buffer in hand when processing it, at a time where
the device can no longer modify it (IOW no TOCTTOU issues either.)

(As an aside, I've wondered about TOCTTOU with PCI, given that IOMMUs
can and will do lazy unmap ... but that's a different discussion.)


> At the time, I didn't see a way that it could be
> fixed, hence this patch.

Yeah I mean, the code isn't great, even if it's not _that_ much, but all
the likely() and things in there don't make it easy to read, and the
buffer size handling seems not immediately clear to me. So I probably
couldn't fix it quickly either, though I haven't even seen the reports.
Maciej seems to think it's fixable, at least. And yeah, we'd want to
actually review/audit that, I suppose.


So if you'd have said something like

   Let's disable the RNDIS driver(s) because there are known exploits
   there, nobody really knows how to fix this, and we need a short-term
   solution until the issues are public and somebody steps up to fix and
   maintain it.

I'd have much less of a problem with that. That's not _great_, but at
least it's honest and realistic. That could give us some time and maybe
then we can get the bug reports public once it's no longer an immediate
threat for all kernels, and go about fixing it with more time, maybe
eventually backporting fixes and reverting the disablement etc.

I guess this is why secret bug reports suck so much :-)

Thanks,
johannes