Bug 7501 - SMB2: CREATE request replies getting mangled.
Summary: SMB2: CREATE request replies getting mangled.
Alias: None
Product: Samba 3.6
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: x64 Other
: P3 critical
Target Milestone: ---
Assignee: Volker Lendecke
QA Contact: Samba QA Contact
Depends on:
Reported: 2010-06-08 12:44 UTC by Ira Cooper
Modified: 2010-06-08 22:42 UTC (History)
1 user (show)

See Also:

proposed patch so create works. (1.21 KB, patch)
2010-06-08 12:45 UTC, Ira Cooper
ira: review? (jra)
Minimal patch ? (536 bytes, patch)
2010-06-08 13:15 UTC, Jeremy Allison
no flags Details
Proposed patch. (3.14 KB, patch)
2010-06-08 16:57 UTC, Jeremy Allison
no flags Details
Refactored patch (4.93 KB, patch)
2010-06-08 19:40 UTC, Jeremy Allison
jra: review? (ira)

Note You need to log in before you can comment on or make changes to this bug.
Description Ira Cooper 2010-06-08 12:44:45 UTC
We are seeing an issue where dup_smb2_req is mangling compound create replies.

Patch enclosed.
Comment 1 Ira Cooper 2010-06-08 12:45:32 UTC
Created attachment 5771 [details]
proposed patch so create works.
Comment 2 Jeremy Allison 2010-06-08 12:52:37 UTC
Hmmmm. Can you let me know how you saw this ? The problem with this patch is that it breaks the boilerplate setup for iovecs i and i+1, which may have dependencies in other places. What do the mangled compound replies look like ? Can you attach a wireshark trace or log ?
Comment 3 Ira Cooper 2010-06-08 12:57:18 UTC
The code for i is untouched...

i+1 and i+2 are changed.  What boilerplate am I impacting?

Comment 4 Jeremy Allison 2010-06-08 13:04:08 UTC
In smbd_smb2_request_setup_out() (which initializes all outvecs) we have:

                outhdr = talloc_zero_array(vector, uint8_t,
                if (outhdr == NULL) {
                        return NT_STATUS_NO_MEMORY;

                outbody = outhdr + SMB2_HDR_BODY;

                current[0].iov_base     = (void *)outhdr;
                current[0].iov_len      = SMB2_HDR_BODY;

                current[1].iov_base     = (void *)outbody;
                current[1].iov_len      = 8;

Plus inside smbd_smb2_request_error_ex() we have the comment:

                /* Allocated size of req->out.vector[i].iov_base
                 * *MUST BE* OUTVEC_ALLOC_SIZE. So we have room for
                 * 1 byte without having to do an alloc.

Also, the code you replaced is also duplicated inside smbd_smb2_request_pending_queue(), so if it's wrong I really want to understand *why* it's wrong :-). I'm not saying your patch is incorrect, I just don't understand why the original code screws up and that scares me :-).

Comment 5 Ira Cooper 2010-06-08 13:12:03 UTC
The boilerplate should be safe here... because the copy will keep the length of the field being copied from.  And that comment should be at LEAST...  i+1 is certainly changed in the code I thought?

I'll see what I can do to get a reproduction.
Comment 6 Jeremy Allison 2010-06-08 13:13:43 UTC
Hang on a minute... I see the problem (I think). How about this as a minimal patch ?

diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index df25570..009cc77 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -551,9 +551,7 @@ static struct smbd_smb2_request *dup_smb2_req(const struct smbd_smb2_request *re
                        outvec[i+2].iov_base = ((uint8_t *)outvec[i].iov_base) +
                                (OUTVEC_ALLOC_SIZE - 1);
                        outvec[i+2].iov_len = 1;
-               } else if (!dup_smb2_vec(outvec,
-                               req->out.vector,
-                               i)) {
+               } else if (!dup_smb2_vec(outvec, req->out.vector, i+2)) {
Comment 7 Jeremy Allison 2010-06-08 13:15:12 UTC
Created attachment 5772 [details]
Minimal patch ?

Arg. Mangled by the text field. This is what I meant..
Comment 8 Jeremy Allison 2010-06-08 13:19:59 UTC
Ok, I'm pretty sure this will fix it - that's the core of the problem (I was duplicating vector i, instead of vector i+2). The other part of your patch (duplicating vector i+1) is correct but I think unnecessary. I'm going to check in the minimal patch under your name, and if it doesn't fix the problem I'll investigate further and possibly add in the second part (once I understand the problem fully :-). But I think this should fix it.

Thanks once again :-) :-). I am very impressed with how much you test our code. Let me know if I can do anything for you guys as thanks.

Comment 9 Ira Cooper 2010-06-08 13:31:16 UTC
Still broken... Do not push.

Comment 10 Jeremy Allison 2010-06-08 13:34:31 UTC
Dam, too late (sorry). Can we chat on the phone ? I really need to understand this one..

Comment 11 Jeremy Allison 2010-06-08 13:41:17 UTC
Ahhhh. *Now* I get it (looking really carefully at the create code...).
This needs more work. Sorry for the early push. I'll credit you with all changes.

Comment 12 Ira Cooper 2010-06-08 13:45:12 UTC
In reading the code... I think my changes are right in spirit.  You shouldn't
be assuming what is in outvec[i] has anything in common with outvec[i+1] or

You may want to assume i+1 has a non-zero length, and that's fine if
initialized that way... but in a general case... i+1 and i+2 can be whatever
the call deems it wants I thought.  Even i is really that way, but by
convention we put the header in there only... But I haven't seen anything
enforce that rule.

The code as is when I read it looks more and more wrong... a simpler check for
non-zero length and if so, just copy the vector seems right.

Comment 13 Jeremy Allison 2010-06-08 16:57:04 UTC
Created attachment 5773 [details]
Proposed patch.

Thanks for being patient with me. This is the patch that should work for all cases. It contains your original fix (with a few extra comments), plus an adjustment to the vector calculations in smbd_smb2_request_pending_queue().

Please let me know if this fixes the problems.

Comment 14 Ira Cooper 2010-06-08 17:42:54 UTC
I'll get it tested tomorrow.

As far as the patch goes:

Now the code uses two techniques, I'd just use one or the other, probably the one in smbd_smb2_request_pending_queue which is more efficient.  (though more confusing.)

Is there a way to make one function that has all this logic in it, instead of duplicating it?

Comment 15 Jeremy Allison 2010-06-08 18:47:13 UTC
Hmmm. Good point, I'll look at that.

Comment 16 Jeremy Allison 2010-06-08 19:40:23 UTC
Created attachment 5774 [details]
Refactored patch

Ok, that was a very good idea of yours :-). Refactored patch abstracting the change out to a function dup_smb2_vec3(), that replaces dup_smb2_vec(). Should fix the bug and is *much* cleaner :-). Thanks !

Please test.

Comment 17 Ira Cooper 2010-06-08 20:58:18 UTC
Patch looks good, and works.  (assuming that's what is on master now :) )

Comment 18 Jeremy Allison 2010-06-08 22:42:05 UTC
Yep - that's what is in master now. Closing this one out - thanks a *lot* for the help !