PATCH: ctdb: buffer write beyond limits

Volker Lendecke Volker.Lendecke at SerNet.DE
Tue Feb 19 10:53:52 UTC 2019


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.

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: 0551-370000-0, mailto:kontakt at sernet.de
Gesch.F.: Dr. Johannes Loxen und Reinhild Jung
AG Göttingen: HR-B 2816 - http://www.sernet.de
-------------- next part --------------
 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..cab9cc94a26 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 = 0;
 	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) {
+		DEBUG(DEBUG_ERR, ("buffer overflow\n"));
+		goto failed;
+	}
+	if (start_offset > queue->buffer.size) {
+		DEBUG(DEBUG_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,


More information about the samba-technical mailing list