Padding byte in cifs readx response

Christof Schmitt cs at samba.org
Wed Aug 6 15:38:39 MDT 2014


On Fri, Aug 01, 2014 at 03:19:05PM -0700, Christof Schmitt wrote:
> 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.

Here is some progress. I still get a selftest failure with a large read
request, so something must be wrong with the outbuf, but the tests i
added succeed.  I would like to get some feedback if my patches are the
right approach, or if adding the padding byte can be simplified.

Christof
-------------- next part --------------
>From 1384f67031251d94823b5fc267a64027f62877f7 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Tue, 29 Jul 2014 13:39:57 -0700
Subject: [PATCH 1/6] torture: Add test for 16 bit alignment of readx data

MS-CIFS requires a one byte pad to guarantee 16 bit alignment of the
data:

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.
---
 source4/libcli/raw/interfaces.h   |    2 ++
 source4/libcli/raw/rawreadwrite.c |    2 ++
 source4/torture/raw/read.c        |    6 ++++++
 3 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/source4/libcli/raw/interfaces.h b/source4/libcli/raw/interfaces.h
index 12c0377..9003c12 100644
--- a/source4/libcli/raw/interfaces.h
+++ b/source4/libcli/raw/interfaces.h
@@ -1809,6 +1809,8 @@ union smb_read {
 			uint16_t remaining;
 			uint16_t compaction_mode;
 			uint32_t nread;
+			uint16_t flags2;
+			uint16_t data_offset;
 		} out;
 	} readx, generic;
 
diff --git a/source4/libcli/raw/rawreadwrite.c b/source4/libcli/raw/rawreadwrite.c
index d3f5518..fb44ba4 100644
--- a/source4/libcli/raw/rawreadwrite.c
+++ b/source4/libcli/raw/rawreadwrite.c
@@ -155,6 +155,8 @@ _PUBLIC_ NTSTATUS smb_raw_read_recv(struct smbcli_request *req, union smb_read *
 		parms->readx.out.remaining       = SVAL(req->in.vwv, VWV(2));
 		parms->readx.out.compaction_mode = SVAL(req->in.vwv, VWV(3));
 		parms->readx.out.nread = SVAL(req->in.vwv, VWV(5));
+		parms->readx.out.flags2 = req->flags2;
+		parms->readx.out.data_offset = SVAL(req->in.vwv, VWV(6));
 
 		/* handle oversize replies for non-chained readx replies with
 		   CAP_LARGE_READX. The snia spec has must to answer for. */
diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index 59089bf..148df3b 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -435,6 +435,12 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli)
 		goto done;
 	}
 
+	if ((io.readx.out.flags2 & FLAGS2_UNICODE_STRINGS) &&
+	    (io.readx.out.data_offset % 2 != 0)) {
+		ret = false;
+		torture_fail_goto(tctx, done, "data not 16 bit aligned\n");
+	}
+
 	printf("Trying short read\n");
 	io.readx.in.offset = 1;
 	io.readx.in.mincnt = strlen(test_data);
-- 
1.7.1


>From df59f9b9b3446855df5609ff9930580f08720094 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Aug 2014 11:20:57 -0700
Subject: [PATCH 2/6] debug

---
 source3/smbd/reply.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index d54326a..2b57540 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3867,6 +3867,10 @@ nosendfile_read:
 
 	DEBUG(3, ("send_file_readX %s max=%d nread=%d\n",
 		  fsp_fnum_dbg(fsp), (int)smb_maxcnt, (int)nread));
+
+	DEBUG(0, ("read and x outbuf: %s\n",
+		  hex_encode_talloc(talloc_tos(), req->outbuf,
+				    smb_len(req->outbuf))));
 	return;
 
  strict_unlock:
-- 
1.7.1


>From 33f4b45c5440ea49696e5d7ae657afb74fb3adfe Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Aug 2014 13:29:24 -0700
Subject: [PATCH 3/6] incomplete: Add padding byte to non-sendfile readx response

This still fails selftest:

[1/3 in 0s] samba3.raw.read(s3dc)
Testing RAW_READ_READX
Trying empty file read
Trying zero file read
Trying bad fnum
Trying small read
Trying short read
Trying max offset
Trying mincnt past EOF
Trying page sized read
Trying page + 1 sized read (check alignment)
Buffer incorrect at line 512! ofs=4096 v1=0x20 v2=0x73
UNEXPECTED(failure): samba3.raw.read.readx(s3dc)
REASON: _StringException: _StringException:
../source4/torture/raw/read.c:512: buffer check failed
envlog: SMBD LOG of: LOCALS3DC2
---
 source3/smbd/process.c     |   28 ++++++++++++++++++++++++----
 source3/smbd/proto.h       |    2 ++
 source3/smbd/reply.c       |   11 ++++++-----
 source4/torture/raw/read.c |    2 +-
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 127bbb3..70b1a06 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,13 @@ 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);
+
+	if (pad) {
+		smb_setlen(*outbuf, smb_size + num_words*2 + num_bytes - 4 + 1);
+	}
+
 	/*
 	 * Zero out the word area, the caller has to take care of the bcc area
 	 * himself
@@ -1378,13 +1388,23 @@ static bool create_outbuf(TALLOC_CTX *mem_ctx, struct smb_request *req,
 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)) {
+	if (!create_outbuf(req, req, (const char *)req->inbuf, &outbuf,
+			   num_words, 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 +3059,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..2ad7e24 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -769,6 +769,8 @@ 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 2b57540..5f9d5fc 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3674,11 +3674,12 @@ 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. */
-	_smb_setlen_large(outbuf,(smb_size + 12*2 + smb_maxcnt - 4));
+	_smb_setlen_large(outbuf,(smb_size + 12*2 + smb_maxcnt - 4 + 1));
 	return outsize;
 }
 
@@ -3714,7 +3715,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 	    (fsp->base_fsp == NULL) &&
 	    (fsp->wcp == NULL) &&
 	    lp_use_sendfile(SNUM(conn), req->sconn->smb1.signing_state) ) {
-		uint8 headerbuf[smb_size + 12 * 2];
+		uint8 headerbuf[smb_size + 12 * 2 + 1];
 		DATA_BLOB header;
 
 		if(fsp_stat(fsp) == -1) {
@@ -3812,7 +3813,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
 normal_read:
 
 	if ((smb_maxcnt & 0xFF0000) > 0x10000) {
-		uint8 headerbuf[smb_size + 2*12];
+		uint8 headerbuf[smb_size + 2*12 + 1];
 
 		construct_reply_common_req(req, (char *)headerbuf);
 		setup_readX_header(req, (char *)headerbuf, smb_maxcnt);
@@ -3853,7 +3854,7 @@ nosendfile_read:
 	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
 	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
 
-	nread = read_file(fsp, smb_buf(req->outbuf), startpos, smb_maxcnt);
+	nread = read_file(fsp, smb_buf(req->outbuf) + 1, startpos, smb_maxcnt);
 	saved_errno = errno;
 
 	SMB_VFS_STRICT_UNLOCK(conn, fsp, &lock);
diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c
index 148df3b..711feec 100644
--- a/source4/torture/raw/read.c
+++ b/source4/torture/raw/read.c
@@ -44,7 +44,7 @@
 #define CHECK_BUFFER(buf, seed, len) do { \
 	if (!check_buffer(buf, seed, len, __LINE__)) { \
 		ret = false; \
-		goto done; \
+		torture_fail_goto(tctx, done, "buffer check failed\n"); \
 	}} while (0)
 
 #define BASEDIR "\\testread"
-- 
1.7.1


>From 73a07d2acb83d4da16c55b0de87b7e3da52a1909 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Aug 2014 13:53:10 -0700
Subject: [PATCH 4/6] smbd: Add padding byte to readx response from rpc requests

---
 source3/smbd/pipes.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/source3/smbd/pipes.c b/source3/smbd/pipes.c
index f1f61bb..fa0c148 100644
--- a/source3/smbd/pipes.c
+++ b/source3/smbd/pipes.c
@@ -422,7 +422,7 @@ void reply_pipe_read_and_X(struct smb_request *req)
 	state->smb_maxcnt = SVAL(req->vwv+5, 0);
 	state->smb_mincnt = SVAL(req->vwv+6, 0);
 
-	reply_outbuf(req, 12, state->smb_maxcnt);
+	reply_outbuf_pad(req, 12, state->smb_maxcnt);
 	SSVAL(req->outbuf, smb_vwv0, 0xff); /* andx chain ends */
 	SSVAL(req->outbuf, smb_vwv1, 0);    /* no andx offset */
 
@@ -467,7 +467,7 @@ static void pipe_read_andx_done(struct tevent_req *subreq)
 	req->outbuf = state->outbuf;
 	state->outbuf = NULL;
 
-	srv_set_message((char *)req->outbuf, 12, nread, False);
+	srv_set_message((char *)req->outbuf, 12, nread + 1, False);
 
 #if 0
 	/*
-- 
1.7.1


>From b78e911ab16f463e0dccbfa42b89f732db04d434 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Aug 2014 13:55:50 -0700
Subject: [PATCH 5/6] torture: Also run raw.read against the aio share

---
 source3/selftest/tests.py |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index e9b91d4..2c48b34 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -355,6 +355,10 @@ for t in tests:
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
         plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
+    elif t == "raw.read":
+        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
+        plansmbtorture4testsuite(t, "plugin_s4_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
+        plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
     elif t == "raw.search":
         plansmbtorture4testsuite(t, "s3dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
 # test the dirsort module.
-- 
1.7.1


>From 2c3850032d6e70c4b9308c19c38666a05d938914 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 6 Aug 2014 14:24:33 -0700
Subject: [PATCH 6/6] smbd: Add padding byte to aio readx response

---
 source3/smbd/aio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index 2235c32..12a39f9 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -196,7 +196,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
 	/* The following is safe from integer wrap as we've already checked
 	   smb_maxcnt is 128k or less. Wct is 12 for read replies */
 
-	bufsize = smb_size + 12 * 2 + smb_maxcnt;
+	bufsize = smb_size + 12 * 2 + smb_maxcnt + 1;
 
 	if ((aio_ex = create_aio_extra(NULL, fsp, bufsize)) == NULL) {
 		DEBUG(10,("schedule_aio_read_and_X: malloc fail.\n"));
@@ -221,7 +221,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
 	aio_ex->offset = startpos;
 
 	req = SMB_VFS_PREAD_SEND(aio_ex, fsp->conn->sconn->ev_ctx,
-				 fsp, smb_buf(aio_ex->outbuf.data),
+				 fsp, smb_buf(aio_ex->outbuf.data) + 1,
 				 smb_maxcnt, startpos);
 	if (req == NULL) {
 		DEBUG(0,("schedule_aio_read_and_X: aio_read failed. "
@@ -256,7 +256,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
 	files_struct *fsp = aio_ex->fsp;
 	int outsize;
 	char *outbuf = (char *)aio_ex->outbuf.data;
-	char *data = smb_buf(outbuf);
+	char *data = smb_buf(outbuf) + 1;
 	ssize_t nread;
 	int err;
 
@@ -285,7 +285,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
 		ERROR_NT(map_nt_error_from_unix(err));
 		outsize = srv_set_message(outbuf,0,0,true);
 	} else {
-		outsize = srv_set_message(outbuf, 12, nread, False);
+		outsize = srv_set_message(outbuf, 12, nread + 1, False);
 		SSVAL(outbuf,smb_vwv2, 0xFFFF); /* Remaining - must be * -1. */
 		SSVAL(outbuf,smb_vwv5, nread);
 		SSVAL(outbuf,smb_vwv6, smb_offset(data,outbuf));
-- 
1.7.1



More information about the samba-technical mailing list