io_u_qiter: Fix buffer overrun

PATCH In review Sitsofe Wheeler 2 participants 4 messages Feb 13, 2014

Share & tools

Sitsofe Wheeler

author

2014-02-13 07:06 UTC #0 lore raw

PATCH 1/1 io_u_qiter: Fix buffer overrun

In io_u_queue.h the io_u_qiter macro is loops around io_u_queue structures. The problem comes with the end of loop initialisation:
i++, io_u = (q)->io_us[i]
For example, if io_us consists of one element and i is 0 then after the first iteration is completed i++, io_u = (q)->io_us[i] will access beyond the end of io_us.

Fix this by moving io_u initialisation to the expression part of the for loop (yuck).

Found by Dr Memory.

Signed-off-by Sitsofe Wheeler <[email protected]>

---
 io_u_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

io_u_queue.h +1 -1

index 4f6e8e6..5b6cad0 100644
@@ -29,7 +29,7 @@ static inline int io_u_qempty(struct io_u_queue *q)
 }
 
 #define io_u_qiter(q, io_u, i)	\
-	for (i = 0, io_u = (q)->io_us[0]; i < (q)->nr; i++, io_u = (q)->io_us[i])
+	for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++)
 
 int io_u_qinit(struct io_u_queue *q, unsigned int nr);
 void io_u_qexit(struct io_u_queue *q);

Signature · 4 lines

1.8.5.3

-- 
Sitsofe | http://sucs.org/~sits/

Jens Axboe

2014-02-13 16:17 UTC #1 lore raw

Quoted 22 lines · Sitsofe Wheeler

In io_u_queue.h the io_u_qiter macro is loops around io_u_queue structures. The problem comes with the end of loop initialisation:
i++, io_u = (q)->io_us[i]
For example, if io_us consists of one element and i is 0 then after the first iteration is completed i++, io_u = (q)->io_us[i] will access beyond the end of io_us.
Fix this by moving io_u initialisation to the expression part of the for loop (yuck).
Found by Dr Memory.
Signed-off-by Sitsofe Wheeler <[email protected]>
---
 io_u_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
io_u_queue.h +0 -0
index 4f6e8e6..5b6cad0 100644

io_u_queue.h static inline int io_u_qempty(struct io_u_queue *q) · line 32

Quoted 5 lines

io_u_queue.h +1 -1

@@ -29,7 +29,7 @@ static inline int io_u_qempty(struct io_u_queue *q)
 }
 
 #define io_u_qiter(q, io_u, i)	\
-	for (i = 0, io_u = (q)->io_us[0]; i < (q)->nr; i++, io_u = (q)->io_us[i])
+	for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++)

Initially I didn't see the issue, but then I realized that ->io_us is a pointer to the io_u pointer. So it is an issue. The fix isn't super pretty, but it gets rid of the bug, so I'll apply it. It might be nicer to split it into a top and bottom define.

Signature · 1 line

Jens Axboe

Sitsofe Wheeler

author

2014-02-13 20:05 UTC #2 lore raw

Quoted 5 lines · Jens Axboe

Initially I didn't see the issue, but then I realized that ->io_us is a pointer to the io_u pointer. So it is an issue. The fix isn't super pretty, but it gets rid of the bug, so I'll apply it. It might be nicer to split it into a top and bottom define.

Can you explain this top and bottom define more - would this let me turn it into a while loop?

The only reason for the current abuse was because I couldn't think of a another way to fix it while preserving the macro...

Signature · 1 line

Sitsofe | http://sucs.org/~sits/

Jens Axboe

2014-02-13 20:34 UTC #3 lore raw

Quoted 7 lines · Sitsofe Wheeler

Quoted 5 lines · Jens Axboe
Initially I didn't see the issue, but then I realized that ->io_us is a pointer to the io_u pointer. So it is an issue. The fix isn't super pretty, but it gets rid of the bug, so I'll apply it. It might be nicer to split it into a top and bottom define.

Quoted 5 lines

Can you explain this top and bottom define more - would this let me turn it into a while loop?
The only reason for the current abuse was because I couldn't think of a another way to fix it while preserving the macro...

Basically you turn it into a do-while with two macros. Ala the below, completely untested...

backend.c +2 -2

index 32bc2652bd0b..e355447d1e3a 100644
@@ -255,13 +255,13 @@ static void cleanup_pending_aio(struct thread_data *td)
 		struct io_u *io_u;
 		int i;
 
-		io_u_qiter(&td->io_u_all, io_u, i) {
+		io_u_do_qiter(&td->io_u_all, io_u, i) {
 			if (io_u->flags & IO_U_F_FLIGHT) {
 				r = td->io_ops->cancel(td, io_u);
 				if (!r)
 					put_io_u(td, io_u);
 			}
-		}
+		} while_each_io_u(&td->io_u_all, io_u, i);
 	}
 
 	if (td->cur_depth)

engines/posixaio.c +2 -2

index 2df26af3848e..3b27fcdfcc34 100644
@@ -111,7 +111,7 @@ static int fio_posixaio_getevents(struct thread_data *td, unsigned int min,
 restart:
 	memset(suspend_list, 0, sizeof(*suspend_list));
 	suspend_entries = 0;
-	io_u_qiter(&td->io_u_all, io_u, i) {
+	io_u_do_qiter(&td->io_u_all, io_u, i) {
 		int err;
 
 		if (io_u->seen || !(io_u->flags & IO_U_F_FLIGHT))

@@ -138,7 +138,7 @@ restart:
 			io_u->resid = io_u->xfer_buflen - retval;
 		} else
 			io_u->error = err;
-	}
+	} while_each_io_u(&td->io_u_all, io_u, i);
 
 	if (r >= min)
 		return r;

engines/windowsaio.c +2 -2

index 16df74035f18..c6ff27a4408f 100644
@@ -275,7 +275,7 @@ static int fio_windowsaio_getevents(struct thread_data *td, unsigned int min,
 	}
 
 	do {
-		io_u_qiter(&td->io_u_all, io_u, i) {
+		io_u_do_qiter(&td->io_u_all, io_u, i) {
 			if (!(io_u->flags & IO_U_F_FLIGHT))
 				continue;

@@ -290,7 +290,7 @@ static int fio_windowsaio_getevents(struct thread_data *td, unsigned int min,
 
 			if (dequeued >= min)
 				break;
-		}
+		} while_each_io_u(&td->io_u_all, io_u, i);
 
 		if (dequeued < min) {
 			status = WaitForSingleObject(wd->iocomplete_event, mswait);

io_u_queue.h +8 -2

index 5b6cad0ef173..649dcb5ca67c 100644
@@ -28,8 +28,14 @@ static inline int io_u_qempty(struct io_u_queue *q)
 	return !q->nr;
 }
 
-#define io_u_qiter(q, io_u, i)	\
-	for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++)
+#define io_u_do_qiter(q, io_u, i)				\
+	(i) = 0;						\
+	do {							\
+		(io_u) = (q)->io_us[i];				\
+		(i)++;						\
+
+#define while_each_io_u(q, io_u, i)				\
+	} while ((i) < (q)->nr);				\
 
 int io_u_qinit(struct io_u_queue *q, unsigned int nr);
 void io_u_qexit(struct io_u_queue *q);

Signature · 1 line

Jens Axboe