netdev - Re: [PATCH 1/2 v2] usbnet: fix bad header length bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140210155436.GA4258@lianli>
Date:	Mon, 10 Feb 2014 16:54:36 +0100
From:	Emil Goode <emilgoode@...il.com>
To:	Bjørn Mork <bjorn@...k.no>
Cc:	Oliver Neukum <oliver@...kum.org>,
	"David S. Miller" <davem@...emloft.net>,
	Ming Lei <ming.lei@...onical.com>,
	Mark Brown <broonie@...aro.org>,
	Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Glen Turner <gdt@....id.au>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2 v2] usbnet: fix bad header length bug

On Mon, Feb 10, 2014 at 02:05:20PM +0100, Bjørn Mork wrote:
> Oliver Neukum <oliver@...kum.org> writes:
> > On Mon, 2014-02-10 at 13:00 +0100, Emil Goode wrote:
> >> On Mon, Feb 10, 2014 at 07:40:58AM +0100, Oliver Neukum wrote:
> >
> >> > Well, then how about simply removing the check?
> >> > It seems to have outlived its usefulness.
> >> > 
> >> > 	Regards
> >> > 		Oliver
> >> > 
> >> >
> >> 
> >> I did consider that and I think it is probably the best thing to do.
> >> However, I think the removal of the check could have negative effects
> >> on the other minidrivers, at least the qmi_wwan minidriver explicitly
> >> states that it is depending on this check to be made in rx_complete().
> >
> > <censored>.
> 
> No need to do that.  I had the exact same reaction myself :-)
> 
> Anyway, I put that comment there mostly as a note to myself why the
> check would be redundant at that point.  I don't see any problem with
> removing the generic check in usbnet as long as we add similar checks
> whereever they are needed, like in qmi_wwan.

I think it could be worth the trouble of removing the generic check in
the usbnet module.

I believe that if you define your own rx_fixup callback then the
usbnet module should not make it's own assumptions on what packets 
to discard.

Since the checks that need to be added in various places are all in
the same subsystem I think it could be done in as little as one patch?

> Note that usbnet_skb_return will be one of those places.  It calls
> eth_type_trans() on the skb, so it should verify that the length is at
> least ETH_HLEN first.
> 
> > Oh well. But how about merging it with FLAG_MULTI_PACKET?
> > I really don't want to add more flags. There is a point where enough
> > flags make absurd having a common code. We are closing in on that point.
> 
> Agreed.  I would even say we are past that point...

If nobody have any objections I will try removing the generic check and
introduce checks where nessecary.

Best regards,

Emil Goode
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html