netdev - Re: [PATCH] pegasus usb-net: Fix endianness bugs

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 18 Jun 2009 20:02:42 +0200
From:	Michael Buesch <mb@...sch.de>
To:	Jiri Pirko <jpirko@...hat.com>
Cc:	Petko Manolov <petkan@...rs.sourceforge.net>,
	linux-usb@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] pegasus usb-net: Fix endianness bugs

On Thursday 18 June 2009 19:46:22 Jiri Pirko wrote:
> >--- linux-2.6.30.orig/drivers/net/usb/pegasus.c
> >+++ linux-2.6.30/drivers/net/usb/pegasus.c
> >@@ -297,7 +297,7 @@ static int update_eth_regs_async(pegasus
> > 
> > 	pegasus->dr.bRequestType = PEGASUS_REQT_WRITE;
> > 	pegasus->dr.bRequest = PEGASUS_REQ_SET_REGS;
> >-	pegasus->dr.wValue = 0;
> >+	pegasus->dr.wValue = cpu_to_le16(0);
> Is this necessary? I mean zero is still zero :-)

Well, yes. However wValue is a little endian variable and 0 is CPU endian.
The fact that 0 is represented with the same bits in LE and BE doesn't really
matter. It documents the fact that we really require this to be a LE value.
GCC will recognize it and optimize it away, so it's not a runtime issue.
And  wValue = cpu_to_le16(0)  is used in the rest of the driver, too. This is the
only place that doesn't use it. So let's fix it. If only for consistency.

> >+	u8 interval;
> > 
> >-	read_eprom_word(pegasus, 4, (__u16 *) data);
> >+	read_eprom_word(pegasus, 4, &data);
> >+	interval = data >> 8;
> > 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
> >-		if (data[1] < 0x80) {
> >+		if (interval < 0x80) {
> > 			if (netif_msg_timer(pegasus))
> > 				dev_info(&pegasus->intf->dev, "intr interval "
> > 					"changed from %ums to %ums\n",
> >-					data[1], 0x80);
> >-			data[1] = 0x80;
> >+					interval, 0x80);
> >+			interval = 0x80;
> >+			data = (data & 0x00FF) | ((u16)interval << 8);
>                                                   ^^^^^ you do not need this
> 							  typecast

Hm, well. The cast is there because "interval" is an 8bit variable.
I think the behavior of C is machine dependent in such a situation.
But as Linux doesn't run on anything with less than 32bit registers, it probably
doesn't matter.

-- 
Greetings, Michael.
--
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