Padding byte in cifs readx response
Volker Lendecke
Volker.Lendecke at SerNet.DE
Thu Aug 7 08:43:49 MDT 2014
On Wed, Aug 06, 2014 at 02:38:39PM -0700, Christof Schmitt wrote:
> 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.
Find some comments attached. My main comment is: I'd like to avoid passing
down a boolean to create_outbuf and handle this by allocating another
byte at a higher level. Apart from that, I think this will inevitably
be dirty, our SMB1 read&x path just has to cover a lot of situations
where the data comes from.
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From c2f64d945ad4f5f24baa6b9f31ebe8b0c9829dc1 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] VL: 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.
VL: I'd split this up into two, but that's just cosmetics. One for the
_recv function and one for the test itself.
---
source4/libcli/raw/interfaces.h | 2 ++
source4/libcli/raw/rawreadwrite.c | 2 ++
source4/torture/raw/read.c | 6 ++++++
3 files changed, 10 insertions(+)
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.8.1.2
From 35bbf9f33f8346262fd9dba2ea8327a2fc8a7d48 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 file changed, 4 insertions(+)
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 6c66414..e24fbd1 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3900,6 +3900,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.8.1.2
From d6949baa742a310dbe7a82ab060cd6fbba8e4ef9 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] VL: 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
VL: Question: Is it possible to leave alone and in the higher levels
calling this just let it allocate one more byte? This only affects read&x,
all others are unchanged, right?
---
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 7148462..3a39429 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -1344,10 +1344,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.
@@ -1371,7 +1375,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
@@ -1386,13 +1396,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.
@@ -3058,7 +3078,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 22cd921..1bb85a0 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -771,6 +771,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 e24fbd1..d1afcf7 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3688,11 +3688,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;
}
@@ -3729,7 +3730,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), xconn->smb1.signing_state) ) {
- uint8 headerbuf[smb_size + 12 * 2];
+ uint8 headerbuf[smb_size + 12 * 2 + 1];
DATA_BLOB header;
if(fsp_stat(fsp) == -1) {
@@ -3843,7 +3844,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];
ssize_t ret;
construct_reply_common_req(req, (char *)headerbuf);
@@ -3886,7 +3887,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.8.1.2
From 992322ff599b673fc51a3af53e25ca445e386c04 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 file 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.8.1.2
From 952a6fb2e7399b82e3c926e01eea671b02182972 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 file changed, 4 insertions(+)
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.8.1.2
From 069e303bb15ce2b72b41ac23d17ccddf9d4b0d5c 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 file 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.8.1.2
More information about the samba-technical
mailing list