Bug 11115 - Unaligned memory accesses in CIFS
Summary: Unaligned memory accesses in CIFS
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: CIFS (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Jeff Layton
URL:
Keywords:
: 14569 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-07-18 06:46 UTC by John Voltz
Modified: 2011-08-14 23:58 UTC (History)
3 users (show)

See Also:
Kernel Version: 2.6.25.10
Subsystem:
Regression: No
Bisected commit-id:


Attachments
patch for unaligned memory access in CIFS (13.72 KB, patch)
2008-07-18 06:50 UTC, John Voltz
Details | Diff
More unaligned access fixes in NLS (19.39 KB, patch)
2008-07-27 12:09 UTC, John Voltz
Details | Diff
patch -- fix up some unaligned accesses in cifs (24.69 KB, patch)
2011-01-15 15:23 UTC, Jeff Layton
Details | Diff

Description John Voltz 2008-07-18 06:46:35 UTC
Latest working kernel version: 2.6.25.10
Earliest failing kernel version: 2.6.25.10
Distribution: buildroot
Hardware Environment: avr32
Software Environment: buildroot
Problem Description: CIFS contains code that can cause unaligned memory accesses

Steps to reproduce: on avr32: mount -t cifs //host/share /mnt -o user=me,password=duh

I have a patch for it.
Comment 1 John Voltz 2008-07-18 06:50:09 UTC
Created attachment 16875 [details]
patch for unaligned memory access in CIFS

I have only tested the patch on AVR32, and it works where it previously would cause an exception and fail.
Comment 2 John Voltz 2008-07-27 12:09:34 UTC
Created attachment 17003 [details]
More unaligned access fixes in NLS
Comment 3 Jeff Layton 2009-02-17 03:57:23 UTC
I've never run across an avr32 machine, but I've seen similar sorts of alignment
warnings pop out of ia64 machines. I think ia64 can cope with them but
it's a performance hit.

Here's what the avr32 manual says:

-----------------[snip]----------------
AVR32 can access data of size byte, halfword, word and doubleword using
dedicated instructions. The memory system can support unaligned
accesses for selected load/store instructions in some implementations.
Any other unaligned access will cause an address exception.
-----------------[snip]----------------

I assume that we're tripping these address exceptions somehow here?

I'm not thrilled with the approach in the patches. The double copying
seems icky. It seems like there ought to be better ways to do this that
wouldn't penalize arches that handle these unaligned accesses w/o issue,
but I don't know how right offhand.
Comment 4 Steve French 2009-03-02 11:19:31 UTC
Would like to see how this is handled in other components too ...
Comment 5 Jeff Layton 2010-12-22 15:44:00 UTC
John, I've had someone open a RH bug for a similar problem and am interested to see if we can also bring this one to resolution. Do you happen to have a more current patchset for this?

Also, is there a reason you decided to just use memcpy to copy the data to temporary variables instead of just using the get_unaligned/put_unaligned macros?
Comment 6 Jeff Layton 2011-01-15 13:16:49 UTC
*** Bug 14569 has been marked as a duplicate of this bug. ***
Comment 7 Jeff Layton 2011-01-15 15:23:44 UTC
Created attachment 43692 [details]
patch -- fix up some unaligned accesses in cifs

This is a preliminary patchset, tested on x86_64 and doesn't seem to break anything...

This first stops the current practice of "converting" the bytecount in a received packet to host-endian format. It then turns the BCC and BCC_LE macros into inlined functions that use get/put_unaligned_le16 to fetch and store the it. Several other places in validate_t2 pointed out by John are also converted to use get_unaligned_le16.

For the unicode functions, I've converted them to pass a __u16 stack variable to
char2uni. That's then copied to the destination buffer using put_unaligned_le16. With that change, we shouldn't need to change any NLS code.

I don't have easy access to arches that have the problems that this is intended to fix. If someone here could help test and verify this patch that would be a great help.

Also note that this is probably not a comprehensive fix for CIFS. There are a number places in the code that simply seem to do a "le16_to_cpu" or "cpu_to_le16". It's possible some of those may also need fixing, but we'll need to look at them on a case-by-case basis.
Comment 8 Jeff Layton 2011-01-17 13:08:16 UTC
Actually, this patch will probably break big-endian arches. There are a number of places in the cifs code that access the ByteCount directly and hence rely on it to be host-endian.

I'll probably have to step back and take another approach to this...
Comment 9 Jeff Layton 2011-01-18 20:54:25 UTC
Ok, I've respun the patchset and posted it upstream. Comments and testing would be appreciated:

    http://thread.gmane.org/gmane.linux.kernel.cifs/2308

...I don't think this will be a complete solution for CIFS on avr32 however. There are a number of places that access fields directly in a received buffer or do the same in a buffer that it's going to emit. Those will likely need to be fixed in a second pass.
Comment 10 Jeff Layton 2011-08-14 23:58:22 UTC
I think this is mostly fixed now upstream. There may be some other places that need work here, but no one has reported them. Please reopen this if you see any more problems of this sort in current mainline kernels.

Note You need to log in before you can comment on or make changes to this bug.