Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Sat, 21 Sep 2019 14:54:03 +0200, Bjørn Mork wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> > On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote:  
> >> Endpoints with zero wMaxPacketSize are not usable for transferring
> >> data. Ignore such endpoints when looking for valid in, out and
> >> status pipes, to make the drivers more robust against invalid and
> >> meaningless descriptors.
> >> 
> >> The wMaxPacketSize of these endpoints are used for memory allocations
> >> and as divisors in many usbnet minidrivers. Avoiding zero is therefore
> >> critical.
> >> 
> >> Signed-off-by: Bjørn Mork <bjorn@mork.no>  
> >
> > Fixes tag would be useful. I'm not sure how far into stable we should
> > backport this.  
> 
> That would be commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so I don't think
> a Fixes tag is very useful...

It's slightly useful to add it anyway, IMHO, even if it's 2.6.12,
because it may save people processing the patch checking how far
it applies. You already did the research, anyway.

Granted, that's a little `process-centric`, rather than `merit-centric`
view.

> I haven't verified how deep into the code you have been able to get with
> wMaxPacketSize being zero.  But I don't think there ever has been much
> protection since it's so obviously "insane".  There was no point in
> protecting against this as long as we considered the USB port a security
> barrier.
> 
> I see that the v2.6.12-rc2 version of drivers/usb/net/usbnet.c (sic)
> already had this in it's genelink_tx_fixup():
<snip>

Thanks for the detailed analysis!

> So, to summarize:  I believe the fix is valid for all stable versions.
> 
> I'll leave it up to the more competent stable maintainers to decide how
> many, if any, it should be backported to.  I will not cry if the answer
> is none.

Right, I'll put it in the stable queue, we'll see if it passes Dave's 
and Greg's filters :)

> > Is this something that occurs on real devices or protection from
> > malicious ones?  
> 
> Only malicious ones AFAICS.
> 
> I don't necessarily agree, but I believe the current policy makes this a
> "security" issue.  CVEs have previously been allocated for similar
> crashes triggered by buggy USB descriptors.  For some reason we are
> supposed to protect the system against *some* types of malicious
> hardware.

I see, the patch is fairly intrusive and very unlikely to cause to lead
to regressions on real devices, so regardless of the practical impact
shouldn't hurt.

> I am looking forward to the fixes coming up next to protect against
> malicious CPUs and microcode ;-)

I hope not before we retire..

Applied, queued for stable, thank you!