linux-kernel - RE: [PATCH] USB/ISP1760: Fix for unaligned exceptions

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]

Date:	Tue, 18 Nov 2008 15:41:01 -0000
From:	"Hennerich, Michael" <Michael.Hennerich@...log.com>
To:	"Sebastian Andrzej Siewior" <bigeasy@...utronix.de>,
	"Bryan Wu" <cooloney@...nel.org>
Cc:	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	"Michael Hennerich" <michael.hennerich@...log.com>
Subject: RE: [PATCH] USB/ISP1760: Fix for unaligned exceptions

Sebastian,

>The link [1] you sent me reports an unaligned access which occurred
here.
>So I thing the access to *src should be either a get_unaligned helper
or a
>byte read loop like I did it in the read path.

It's not just that single spot.
I've seen unaligned pointers with count > 3 coming from various drivers.

Here just two examples:

1) The generic Bluetooth USB driver: CONFIG_BT_HCIUSB  
Bluez-utils: hcitool scan:

priv_write_copy: src = 00efaa09, dst = 203c1200, len = 13

Full trace attached.

2) RTL8150 based USB Ethernet adapter: CONFIG_USB_RTL8150
dhcpcd:

priv_read_copy: src = 00ea4812, dst = 203d8000, len = 64

This trace was taken with the unaligned workaround for lengths < 4.

I wonder if it's only us (NOMMU) seeing these odd aligned buffers?

-Michael



>-----Original Message-----
>From: Sebastian Andrzej Siewior [mailto:bigeasy@...utronix.de]
>Sent: Tuesday, November 18, 2008 11:52 AM
>To: Bryan Wu
>Cc: linux-usb@...r.kernel.org; linux-kernel@...r.kernel.org; Michael
>Hennerich
>Subject: Re: [PATCH] USB/ISP1760: Fix for unaligned exceptions
>
>Bryan Wu wrote:
>> From: Michael Hennerich <michael.hennerich@...log.com>
>> Signed-off-by: Michael Hennerich <michael.hennerich@...log.com>
>> Signed-off-by: Bryan Wu <cooloney@...nel.org>
>> ---
>>  drivers/usb/host/isp1760-hcd.c |   67
++++++++++++++++++++++++++++------
>-----
>>  1 files changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/usb/host/isp1760-hcd.c
b/drivers/usb/host/isp1760-
>hcd.c
>> index 8017f1c..00bece2 100644
>> --- a/drivers/usb/host/isp1760-hcd.c
>> +++ b/drivers/usb/host/isp1760-hcd.c
>> @@ -136,12 +136,21 @@ static void priv_read_copy(struct isp1760_hcd
>*priv, u32 *src,
>>  		return;
>>  	}
>>
>> -	while (len >= 4) {
>> -		*src = __raw_readl(dst);
>> -		len -= 4;
>> -		src++;
>> -		dst++;
>> -	}
>> +	if (unlikely((u32)src & 0x3)) {
>> +		while (len >= 4) {
>> +			put_unaligned(__raw_readl(dst), src);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +	} else {
>> +		while (len >= 4) {
>> +			*src = __raw_readl(dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +	}
>>
>>  	if (!len)
>>  		return;
>> @@ -159,25 +168,45 @@ static void priv_read_copy(struct isp1760_hcd
>*priv, u32 *src,
>>  		len--;
>>  		buff8++;
>>  	}
>> +
>>  }
>>
>>  static void priv_write_copy(const struct isp1760_hcd *priv, const
u32
>*src,
>>  		__u32 __iomem *dst, u32 len)
>>  {
>> -	while (len >= 4) {
>> -		__raw_writel(*src, dst);
>> -		len -= 4;
>> -		src++;
>> -		dst++;
>> -	}
>>
>> -	if (!len)
>> -		return;
>> -	/* in case we have 3, 2 or 1 by left. The buffer is allocated
and the
>> -	 * extra bytes should not be read by the HW
>> -	 */
>> -
>> -	__raw_writel(*src, dst);
>The link [1] you sent me reports an unaligned access which occurred
here.
>So I thing the access to *src should be either a get_unaligned helper
or a
>byte read loop like I did it in the read path.
>The r8a66597 is doing the same thing (as you suggest) for one SH
machine.
>However, I'm not convinced to fix it that way: The buffer should be
>properly aligned by the driver. Unless there is HW which requires
>unaligned data, I would prefer just to fix this unaligned access.
>According to the thread in [1] a similar patch fixed it for the user
until
>he dropped into another bug.
>
>> +	if (unlikely((u32)src & 0x3)) {
>> +		while (len >= 4) {
>> +			__raw_writel(get_unaligned(src), dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +
>> +		if (!len)
>> +			return;
>> +		/* in case we have 3, 2 or 1 by left. The buffer is
allocated
>and the
>> +		 * extra bytes should not be read by the HW
>> +		 */
>> +
>> +		__raw_writel(get_unaligned(src), dst);
>> +
>> +	} else{
>> +		while (len >= 4) {
>> +			__raw_writel(*src, dst);
>> +			len -= 4;
>> +			src++;
>> +			dst++;
>> +		}
>> +
>> +		if (!len)
>> +			return;
>> +		/* in case we have 3, 2 or 1 by left. The buffer is
allocated
>and the
>> +		 * extra bytes should not be read by the HW
>> +		 */
>> +
>> +		__raw_writel(*src, dst);
>> +	}
>>  }
>>
>>  /* memory management of the 60kb on the chip from 0x1000 to 0xffff
*/
>
>[1]
>http://blackfin.uclinux.org/gf/project/uclinux-
>dist/forum/?action=ForumBrowse&_forum_action=MessageReply&message_id=64
889
>
>Sebastian

Download attachment "dumps.tar.bz2" of type "application/octet-stream" (5423 bytes)