Padding byte in cifs readx response

[Christof Schmitt]

On Wed, Jul 30, 2014 at 05:09:45PM -0700, Jeremy Allison wrote:
> On Wed, Jul 30, 2014 at 04:34:01PM -0700, Christof Schmitt wrote:
> > It was pointed out to me that a padding byte in the readx response is
> > missing:
> > 
> > http://msdn.microsoft.com/en-us/library/ee441872.aspx
> >  Pad (1 byte): This field is optional. When using the NT LAN Manager
> > dialect, this field can be used to align the Data field to a 16-bit
> > boundary relative to the start of the SMB Header. If Unicode strings are
> > being used, this field MUST be present. When used, this field MUST be
> > one padding byte long.
> > 
> > I wrote a simple test for this, and it succeeds against a Windows 7 and
> > fails in
> > make test TESTS=raw.read
> > 
> > UNEXPECTED(failure): samba3.raw.read.readx(s3dc)
> > REASON: _StringException: _StringException:
> > ../source4/torture/raw/read.c:441: data not 16 bit aligned
> > envlog: SMBD LOG of: LOCALS3DC2
> 
> Probably missing in smbd in order to make the sendfile
> code easier....
> 
> I'm guessing the Windows client works fine without it,
> but probably a good thing to fix so long as it doesn't
> make smbd horribly slower in this case.

I am trying to fix the non-sendfile codepath first, but i seem to be
missing something. The attached patch is my current (failing) attempt, i
will continue with this next week.

Christof
-------------- next part --------------
>From e648ede0ff875a92a1eeea0cc7d404da78bc819d Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 1 Aug 2014 15:17:04 -0700
Subject: [PATCH] wip

---
 source3/smbd/process.c |   29 +++++++++++++++++++++++++----
 source3/smbd/proto.h   |    1 +
 source3/smbd/reply.c   |    6 ++++--
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 127bbb3..5d20516 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -1336,10 +1336,14 @@ static const struct smb_message_struct {
 
 static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
 			  const char *inbuf, char **outbuf, uint8_t num_words,
-			  uint32_t num_bytes)
+			  uint32_t num_bytes, bool pad)
 {
 	size_t smb_len = MIN_SMB_SIZE + VWV(num_words) + num_bytes;
 
+	if (pad) {
+		smb_len += 1;
+	}
+
 	/*
 	 * Protect against integer wrap.
 	 * The SMB layer reply can be up to 0xFFFFFF bytes.
@@ -1363,7 +1367,15 @@ static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
 	}
 
 	construct_reply_common(req, inbuf, *outbuf);
-	srv_set_message(*outbuf, num_words, num_bytes, false);
+
+	SCVAL(*outbuf,smb_wct,num_words);
+	SSVAL(*outbuf,smb_vwv + num_words*SIZEOFWORD,num_bytes);
+	if (pad) {
+		smb_setlen(*outbuf,(smb_size + num_words*2 + 1 + num_bytes - 4));
+	} else {
+		smb_setlen(*outbuf,(smb_size + num_words*2 + num_bytes - 4));
+	}
+
 	/*
 	 * Zero out the word area, the caller has to take care of the bcc area
 	 * himself
@@ -1379,12 +1391,21 @@ void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes)
 {
 	char *outbuf;
 	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf, num_words,
-			   num_bytes)) {
+			   num_bytes, false)) {
 		smb_panic("could not allocate output buffer\n");
 	}
 	req->outbuf = (uint8_t *)outbuf;
 }
 
+void reply_outbuf_pad(struct smb_request *req, uint8 num_words, uint32 num_bytes)
+{
+	char *outbuf;
+	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf, num_words,
+			   num_bytes, true)) {
+		smb_panic("could not allocate output buffer\n");
+	}
+	req->outbuf = (uint8_t *)outbuf;
+}
 
 /*******************************************************************
  Dump a packet to a file.
@@ -3039,7 +3060,7 @@ static bool smbd_echo_reply(struct smbd_echo_state *state,
 	}
 
 	if (!create_outbuf(talloc_tos(), &req, (const char *)req.inbuf, &outbuf,
-			   1, req.buflen)) {
+			   1, req.buflen, false)) {
 		DEBUG(10, ("create_outbuf failed\n"));
 		return false;
 	}
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index eab05c2..ed777e9 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -769,6 +769,7 @@ bool push_deferred_open_message_smb(struct smb_request *req,
 			       struct deferred_open_record *open_rec);
 NTSTATUS allow_new_trans(struct trans_state *list, uint64_t mid);
 void reply_outbuf(struct smb_request *req, uint8 num_words, uint32 num_bytes);
+void reply_outbuf_pad(struct smb_request *req, uint8 num_words, uint32 num_bytes);
 void smb_request_done(struct smb_request *req);
 const char *smb_fn_name(int type);
 void add_to_common_flags2(uint32 v);
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index d54326a..1751d27 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3674,7 +3674,8 @@ static int setup_readX_header(struct smb_request *req, char *outbuf,
 	      (smb_wct - 4)	/* offset from smb header to wct */
 	      + 1 		/* the wct field */
 	      + 12 * sizeof(uint16_t) /* vwv */
-	      + 2);		/* the buflen field */
+	      + 2		/* the buflen field */
+	      + 1);		/* padding byte */
 	SSVAL(outbuf,smb_vwv7,(smb_maxcnt >> 16));
 	SSVAL(outbuf,smb_vwv11,smb_maxcnt);
 	/* Reset the outgoing length, set_message truncates at 0x1FFFF. */
@@ -3849,7 +3850,8 @@ normal_read:
 
 nosendfile_read:
 
-	reply_outbuf(req, 12, smb_maxcnt);
+	reply_outbuf_pad(req, 12, smb_maxcnt);
+
 	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
 	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
 
-- 
1.7.1