Bug 11277 - Compound read responses must be padded to 8 bytes
Summary: Compound read responses must be padded to 8 bytes
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-17 16:42 UTC by Ralph Böhme
Modified: 2015-06-06 20:05 UTC (History)
2 users (show)

See Also:


Attachments
Patch and torture test (10.25 KB, patch)
2015-05-17 16:45 UTC, Ralph Böhme
no flags Details
Patch and torture test (10.17 KB, patch)
2015-05-17 16:49 UTC, Ralph Böhme
no flags Details
Compound read from Win2012 (8.94 KB, application/octet-stream)
2015-05-18 04:29 UTC, Ralph Böhme
no flags Details
Patch for master (8.15 KB, patch)
2015-05-28 08:50 UTC, Ralph Böhme
no flags Details
Patch for 4.2 cherry-picked from master (10.95 KB, patch)
2015-05-29 06:54 UTC, Ralph Böhme
metze: review+
Details
Patch for 4.1 cherry-picked from master (10.95 KB, patch)
2015-05-29 07:47 UTC, Ralph Böhme
metze: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2015-05-17 16:42:00 UTC
The OS X client kernel panics when doing compound reads from Samba, because it expects the compound read response to be padded to an 8 byte boundary.

Windows client behaviour:
- doesn't use compound reads

Windows SMB server behaviour:
- compound read responses are always padded to an 8 byte boundary

OS X client behaviour:
- compound read requests are not padded against OS X SMB server (AAPL?)
- compound read requests are padded against Windows and Samba
- if compound read request was padded and response is NOT padded, the client kernel panics

OS X SMB server behaviour with smbtorture (ie no AAPL):
- if compound read request is padded, response is padded
- if compound read request is not padded, response is also not padded

For Samba imo this boils down to: employ Windows behaviour of always padding compound read responses, patch to follow, including torture test.

Once I have the bug id, I'll query dochelp for clarification on [MS-SMB2] 3.3.4.1.3 <https://msdn.microsoft.com/en-us/library/cc246720.aspx> where compound responses are discussed.
Comment 1 Ralph Böhme 2015-05-17 16:45:01 UTC
Created attachment 11057 [details]
Patch and torture test
Comment 2 Ralph Böhme 2015-05-17 16:49:19 UTC
Created attachment 11058 [details]
Patch and torture test
Comment 3 Ralph Böhme 2015-05-18 04:29:54 UTC
Created attachment 11063 [details]
Compound read from Win2012

Compound read response is padded to an 8 byte boundary.
Comment 4 Ralph Böhme 2015-05-27 11:59:08 UTC
dochelp confirmed that "all the compound responses sent by Windows servers are 8 byte aligned". MS-SMB2 will be updated with an additional Windows behavior note.
Comment 5 Jeremy Allison 2015-05-28 00:17:13 UTC
Ralph, I want to push this but I think

+       uint32_t next_command_body_ofs, tmp_offset, next_command_ofs = 0;

should move tmp_offset inside the body of:

+       /*
+        * See if we need to recalculate the offset to the next
+        * response, or add padding for the last request in a compound
+        * chain.
+        */
+
+       if ((next_command_body_ofs > 0) ||
+           ((next_command_body_ofs == 0) && (flags & SMB2_HDR_FLAG_CHAINED))) {
+               tmp_offset  = SMB2_HDR_BODY;
+               tmp_offset += SMBD_SMB2_OUT_BODY_LEN(req);
+               tmp_offset += SMBD_SMB2_OUT_DYN_LEN(req);
+
+               if (next_command_body_ofs > 0) {
+                       /*
+                        * We have a next compound, use calculated
+                        * offset and add padding.
+                        */
+                       next_command_ofs = tmp_offset;
+                       pad_size = 8 - (tmp_offset % 8);
+               } else if (flags & SMB2_HDR_FLAG_CHAINED) {
+                       /*
+                        * Last command (next_command_body_ofs == 0)
+                        * in a compound request, add padding to 8
+                        * byte boundary if necessary.  According to
+                        * [MS-SMB2] 3.3.4.1.3, Sending Compounded
+                        * Responses this, this shouldn't be
+                        * necessary, but OS X client crash hard if we
+                        * don't and this is also what Windows 2012
+                        * server does.
+                        */
+                       pad_size = 8 - (tmp_offset % 8);
+               }
        }

as this is the only place that uses it. Can you do that tidyup and re-post the patch ?

Thanks !

Jeremy.
Comment 6 Ralph Böhme 2015-05-28 07:49:20 UTC
Hi Jeremy! metze reviewed the patch and gave some more recommmendations for a proper fix. Will attach an updated patch later on today. Thanks!
Comment 7 Ralph Böhme 2015-05-28 08:50:59 UTC
Created attachment 11099 [details]
Patch for master
Comment 8 Ralph Böhme 2015-05-29 06:54:08 UTC
Created attachment 11103 [details]
Patch for 4.2 cherry-picked from master
Comment 9 Ralph Böhme 2015-05-29 07:47:38 UTC
Created attachment 11104 [details]
Patch for 4.1 cherry-picked from master
Comment 10 Karolin Seeger 2015-06-01 19:13:09 UTC
Pushed to autobuild-v4-[1|2]-test.
Comment 11 Karolin Seeger 2015-06-06 20:05:19 UTC
(In reply to Karolin Seeger from comment #10)
Pushed to both branches.
Closing out bug report.

Thanks!