PATCH: ctdb: buffer write beyond limits

[swen]

On Tue, 2019-02-19 at 11:53 +0100, Volker Lendecke wrote:
> On Tue, Feb 19, 2019 at 11:02:18AM +0100, Volker Lendecke via samba-
> technical wrote:
> > On Tue, Feb 19, 2019 at 10:53:41AM +0100, swen wrote:
> > > the code is right if the following facts are taken into account.
> > > 1. MAX_SIGNED_INT = MAX_UNSIGNED_INT/2 - 1
> > > 2. length is defined as unsigned int and will get assigned at
> > > most MAX_SIGNED_INT(num_ready)
> > > 3. offset is initially zero and will only grow as much as length
> > > is shrinking (=>code)
> > > 	=> therefore, offset + length are at most MAX_SIGNED_INT.
> > > Guaranteed !
> > > 4. If then another (guaranteed positive) signed integer (here
> > > num_ready) is added we simply
> > >    cannot overflow an unsigned int because 
> > >    MAX_SIGNED_INT + MAX_SIGNED_INT = MAX_UNSIGNED_INT - 2
> > > 
> > > Taken those simple maths into account, I hope you can agree to
> > > the code.
> > 
> > No, I do not agree. There is a simple boiler plate to do a checked
> > addition, and a simple boiler plate to do checked subtraction.
> > Please
> > do it that way.
> 
> More explanation: I do not agree because it is really simple to get
> these lines right on their own respect. And having to read a 4-entry
> proof referencing code elsewhere where it's easy to make to code safe
> in an isolated manner.
> 
> The crash was introduced through patches that did not affect this MIN
> statement at all. This means that the environment can change, and the
> assumptions that you refer to in bullet point 3 might change, causing
> the code to crash again. In this kind of code we have to check every
> individual arithmetic operation in isolation.
> 
> Attached find something that I did in 10 minutes. Please crush it if
> you think your approach is better.

Ok, I surrender !

Please review and push if happy.

Cheers Swen

P.S.: Thanks Volker for your help here.

-------------- next part --------------
From 2f44aa2cc2a482d6923f505764689c17fb5b8f18 Mon Sep 17 00:00:00 2001
From: Swen Schillig <swen at linux.ibm.com>
Date: Fri, 15 Feb 2019 14:34:05 +0100
Subject: [PATCH] ctdb: buffer write beyond limits

In order to calculate the number of bytes correctly which
are to be read into the buffer, the buffer.offset must be taken
into account.

This patch fixes a regression introduced by 382705f495dd.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13791

Signed-off-by: Swen Schillig <swen at linux.ibm.com>
---
 ctdb/common/ctdb_io.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/ctdb/common/ctdb_io.c b/ctdb/common/ctdb_io.c
index d86540762ea..c16eb7f67b7 100644
--- a/ctdb/common/ctdb_io.c
+++ b/ctdb/common/ctdb_io.c
@@ -164,6 +164,7 @@ static void queue_io_read(struct ctdb_queue *queue)
 {
 	int num_ready = 0;
 	uint32_t pkt_size = 0;
+	uint32_t start_offset;
 	ssize_t nread;
 	uint8_t *data;
 
@@ -226,7 +227,17 @@ buffer_shift:
 	}
 
 data_read:
-	num_ready = MIN(num_ready, queue->buffer.size - queue->buffer.length);
+	start_offset = queue->buffer.length + queue->buffer.offset;
+	if (start_offset < queue->buffer.length) {
+		DBG_ERR("Buffer overflow\n");
+		goto failed;
+	}
+	if (start_offset > queue->buffer.size) {
+		DBG_ERR("Buffer overflow\n");
+		goto failed;
+	}
+
+	num_ready = MIN(num_ready, queue->buffer.size - start_offset);
 
 	if (num_ready > 0) {
 		nread = sys_read(queue->fd,
-- 
2.20.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190219/939f639f/signature.sig>