netdev - Re: [PATCH] net/usb: Misc. fixes for the LG-VL600 LTE USB modem

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: <20111109185714.GA15884@prolixium.com>
Date:	Wed, 9 Nov 2011 13:57:14 -0500
From:	Mark Kamichoff <prox@...lixium.com>
To:	Dan Williams <dcbw@...hat.com>
Cc:	oliver@...kum.name, gregkh@...e.de, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net/usb: Misc. fixes for the LG-VL600 LTE USB modem

On Wed, Nov 09, 2011 at 11:51:44AM -0600, Dan Williams wrote:
> On Tue, 2011-11-08 at 22:10 -0500, Mark Kamichoff wrote:
> > Add checking for valid magic values (needed for stability in the event
> > corrupted packets are received) and remove some other unneeded checks.
> > Also, fix flagging device as WWAN (Bugzilla bug #39952).
> > 
> > Signed-off-by: Mark Kamichoff <prox@...lixium.com>
> > ---
> >  drivers/net/usb/cdc_ether.c |    2 +-
> >  drivers/net/usb/lg-vl600.c  |   30 ++++++++++++++----------------
> >  2 files changed, 15 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
> > index c924ea2..99ed6eb 100644
> > --- a/drivers/net/usb/cdc_ether.c
> > +++ b/drivers/net/usb/cdc_ether.c
> > @@ -567,7 +567,7 @@ static const struct usb_device_id	products [] = {
> >  {
> >  	USB_DEVICE_AND_INTERFACE_INFO(0x1004, 0x61aa, USB_CLASS_COMM,
> >  			USB_CDC_SUBCLASS_ETHERNET, USB_CDC_PROTO_NONE),
> > -	.driver_info = (unsigned long)&wwan_info,
> > +	.driver_info = 0,
> >  },
> >  
> >  /*
> > diff --git a/drivers/net/usb/lg-vl600.c b/drivers/net/usb/lg-vl600.c
> > index d43db32..b975a39 100644
> > --- a/drivers/net/usb/lg-vl600.c
> > +++ b/drivers/net/usb/lg-vl600.c
> > @@ -144,10 +144,11 @@ static int vl600_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> >  	}
> >  
> >  	frame = (struct vl600_frame_hdr *) buf->data;
> > -	/* NOTE: Should check that frame->magic == 0x53544448?
> > -	 * Otherwise if we receive garbage at the beginning of the frame
> > -	 * we may end up allocating a huge buffer and saving all the
> > -	 * future incoming data into it.  */
> > +	/* Yes, check that frame->magic == 0x53544448 (or 0x44544d48),
> > +	 * otherwise we may run out of memory w/a bad packet */
> > +	if (ntohl(frame->magic) != 0x53544448 &&
> > +			ntohl(frame->magic) != 0x44544d48)
> > +		goto error;
> >  
> >  	if (buf->len < sizeof(*frame) ||
> >  			buf->len != le32_to_cpup(&frame->len)) {
> > @@ -209,8 +210,9 @@ static int vl600_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> >  			 * for IPv6 packets, and set the ethertype to IPv6
> >  			 * (0x86dd) so Linux can understand it.
> >  			 */
> > -			if ((buf->data[sizeof(*ethhdr)] & 0xf0) == 0x60)
> > -				ethhdr->h_proto = __constant_htons(ETH_P_IPV6);
> > +			if ((buf->data[sizeof(*ethhdr)] & 0xf0) == 0x60) {
> > +				ethhdr->h_proto = htons(ETH_P_IPV6);
> > +			}
> 
> This change seems somewhat gratuitous; what's the reason for (a) the
> switch from __constant_htons() to plain htons() and (b) adding the {} ?
> 
> Dan

For (a), it's my understanding that __constant_htons() should be used
only for initializers and htons() used in other cases, since it handles
checking for constants.  I suppose you're right and this is a little
gratuitous, but I wanted to keep things clean.

As far as (b), sorry!  That's an error on my part.  I must have been
practicing another coding style at the time.  The braces certainly
shouldn't be there, let me know if I should resubmit.

- Mark

> 
> >  		}
> >  
> >  		if (count) {
> > @@ -296,6 +298,11 @@ encapsulate:
> >  	 * overwrite the remaining fields.
> >  	 */
> >  	packet = (struct vl600_pkt_hdr *) skb->data;
> > +	/* The VL600 wants IPv6 packets to have an IPv4 ethertype
> > +	 * Since this modem only supports IPv4 and IPv6, just set all
> > +	 * frames to 0x0800 (ETH_P_IP)
> > +	 */
> > +	packet->h_proto = htons(ETH_P_IP);
> >  	memset(&packet->dummy, 0, sizeof(packet->dummy));
> >  	packet->len = cpu_to_le32(orig_len);
> >  
> > @@ -308,21 +315,12 @@ encapsulate:
> >  	if (skb->len < full_len) /* Pad */
> >  		skb_put(skb, full_len - skb->len);
> >  
> > -	/* The VL600 wants IPv6 packets to have an IPv4 ethertype
> > -	 * Check if this is an IPv6 packet, and set the ethertype
> > -	 * to 0x800
> > -	 */
> > -	if ((skb->data[sizeof(struct vl600_pkt_hdr *) + 0x22] & 0xf0) == 0x60) {
> > -		skb->data[sizeof(struct vl600_pkt_hdr *) + 0x20] = 0x08;
> > -		skb->data[sizeof(struct vl600_pkt_hdr *) + 0x21] = 0;
> > -	}
> > -
> >  	return skb;
> >  }
> >  
> >  static const struct driver_info	vl600_info = {
> >  	.description	= "LG VL600 modem",
> > -	.flags		= FLAG_ETHER | FLAG_RX_ASSEMBLE,
> > +	.flags		= FLAG_RX_ASSEMBLE | FLAG_WWAN,
> >  	.bind		= vl600_bind,
> >  	.unbind		= vl600_unbind,
> >  	.status		= usbnet_cdc_status,
> 
> 
> 

-- 
Mark Kamichoff
prox@...lixium.com
http://www.prolixium.com/
--
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