| Message ID | 20240523155932.26393-1-chrubis@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | syscalls/msgstress01: Fix off by one in array access | expand |
Hi, I'd at least add a check that size == data.len + 1. On 23. 05. 24 17:59, Cyril Hrubis wrote: > The size returned from recvmsg() is the size of the payload but the > payload is defined as: > > struct { > char len; > char pbytes[99]; > } data; > > So the lenght of the pbytes is actually one byte shorter than the size > and we access one byte after the array in the comparsion. > > Better fix for this would be removal of the len from the data payload > but since we are close to the release lets do the minimal fix now and do > the cleanup after the release. > > Signed-off-by: Cyril Hrubis <chrubis@suse.cz> > --- > testcases/kernel/syscalls/ipc/msgstress/msgstress01.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > index 5c84957b3..b0d945a11 100644 > --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > @@ -131,7 +131,7 @@ static void reader(const int id, const int pos) > return; > } > > - for (int i = 0; i < size; i++) { > + for (int i = 0; i < msg_recv.data.len; i++) { > if (msg_recv.data.pbytes[i] != buff->msg.data.pbytes[i]) { > tst_res(TFAIL, "Received wrong data at index %d: %x != %x", i, > msg_recv.data.pbytes[i],
Hi Cyril, > - for (int i = 0; i < size; i++) { > + for (int i = 0; i < msg_recv.data.len; i++) { > if (msg_recv.data.pbytes[i] != buff->msg.data.pbytes[i]) { > tst_res(TFAIL, "Received wrong data at index %d: %x != %x", i, > msg_recv.data.pbytes[i], Reviewed-by: Petr Vorel <pvorel@suse.cz> And FYI (old, unimportant warning): musl 32bit complains about off_t being long long int. Not sure if we bother (after the release) to %lld + cast to (long long int). ../../../../../include/tst_safe_macros.h:284:50: warning: format '%ld' expects argument of type 'long int', but argument 11 has type 'off_t' {aka 'long long int'} [-Wformat=] 284 | "mmap(%p, %zu, %s(%x), %d, %d, %ld)", | ~~^ | | | long int | %lld 285 | addr, length, prot_buf, prot, flags, fd, offset); | ~~~~~~ | | | off_t {aka long long int} Kind regards, Petr
Hi Cyril, thanks for a quick fix, merged! Kind regards, Petr
Hi Martin, > Hi, > I'd at least add a check that size == data.len + 1. I'm sorry, I overlooked your point. Would you like to send a patch for it? Kind regards, Petr
Hi! > And FYI (old, unimportant warning): musl 32bit complains about off_t being long > long int. Not sure if we bother (after the release) to %lld + cast to (long long int). > > ../../../../../include/tst_safe_macros.h:284:50: warning: format '%ld' expects argument of type 'long int', but argument 11 has type 'off_t' {aka 'long long int'} [-Wformat=] > 284 | "mmap(%p, %zu, %s(%x), %d, %d, %ld)", > | ~~^ > | | > | long int > | %lld Well, we should print it as lld and cast the off the to (long long int) to make the code correct on 32bit. > 285 | addr, length, prot_buf, prot, flags, fd, offset); > | ~~~~~~ > | | > | off_t {aka long long int} > > Kind regards, > Petr
Hi!
> I'd at least add a check that size == data.len + 1.
Which is not true actually because we always send a 100 bytes of data
regardless the message size, which is probably another oversight.
So let's keep the test as it is for now and I will do more work on it
after the release.
To fix this we would have to do at least, but I do not want to change
the test at this point just before the release:
diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c
index b6a64cf4f..f0da595cd 100644
--- a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c
+++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c
@@ -109,7 +109,7 @@ static void writer(const int id, const int pos)
int iter = num_iterations;
while (--iter >= 0 && !(*stop)) {
- int size = msgsnd(id, &buff->msg, 100, IPC_NOWAIT);
+ int size = msgsnd(id, &buff->msg, buff->msg.data.len + 1, IPC_NOWAIT);
if (size < 0) {
if (errno == EAGAIN) {
@@ -160,6 +160,15 @@ static void reader(const int id, const int pos)
return;
}
+ if (msg_recv.data.len + 1 != size) {
+ tst_res(TFAIL,
+ "Wrong message size have %i expected %i",
+ size, msg_recv.data.len+1);
+ *stop = 1;
+ *fail = 1;
+ return;
+ }
+
for (int i = 0; i < msg_recv.data.len; i++) {
if (msg_recv.data.pbytes[i] != buff->msg.data.pbytes[i]) {
tst_res(TFAIL, "Received wrong data at index %d: %x != %x", i,
> Hi! > > I'd at least add a check that size == data.len + 1. > Which is not true actually because we always send a 100 bytes of data > regardless the message size, which is probably another oversight. > So let's keep the test as it is for now and I will do more work on it > after the release. > To fix this we would have to do at least, but I do not want to change > the test at this point just before the release: Sounds reasonable, I'll check it. But now I'm going to tag the release. > diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > index b6a64cf4f..f0da595cd 100644 > --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c > @@ -109,7 +109,7 @@ static void writer(const int id, const int pos) > int iter = num_iterations; > while (--iter >= 0 && !(*stop)) { > - int size = msgsnd(id, &buff->msg, 100, IPC_NOWAIT); > + int size = msgsnd(id, &buff->msg, buff->msg.data.len + 1, IPC_NOWAIT); > if (size < 0) { > if (errno == EAGAIN) { > @@ -160,6 +160,15 @@ static void reader(const int id, const int pos) > return; > } > + if (msg_recv.data.len + 1 != size) { > + tst_res(TFAIL, > + "Wrong message size have %i expected %i", > + size, msg_recv.data.len+1); > + *stop = 1; > + *fail = 1; > + return; Very nit: tst_res(TFAIL and *stop and *fail assignments could be in some macro (e.g. QUIT(msg, ...) ). Kind regards, Petr > + } > + > for (int i = 0; i < msg_recv.data.len; i++) { > if (msg_recv.data.pbytes[i] != buff->msg.data.pbytes[i]) { > tst_res(TFAIL, "Received wrong data at index %d: %x != %x", i,
On 24. 05. 24 13:33, Cyril Hrubis wrote: > Hi! >> I'd at least add a check that size == data.len + 1. > > Which is not true actually because we always send a 100 bytes of data > regardless the message size, which is probably another oversight. > > So let's keep the test as it is for now and I will do more work on it > after the release. Then you should validate the received length against the send buffer. Without any validation of the received length, there's a possibility of buffer overflow.
Hi! > Then you should validate the received length against the send buffer. > Without any validation of the received length, there's a possibility of > buffer overflow. That is actually being done we compare the received lenght against the original buffer in: ... if (msg_recv.data.len != buff->msg.data.len) { tst_res(TFAIL, "Received the wrong message data length"); ... The buff->msg.data.len is the orignal buffer passed to the msgsnd() so we make sure that the length is fits the buffer. We also clear the buffer before each call, so partial message would fail the test because the comparsion of bytes would fail, which is not ideal, but again I do not want to further change the test, because there is much more to fix...
On 24. 05. 24 13:59, Cyril Hrubis wrote: > Hi! >> Then you should validate the received length against the send buffer. >> Without any validation of the received length, there's a possibility of >> buffer overflow. > > That is actually being done we compare the received lenght against the > original buffer in: > > ... > > if (msg_recv.data.len != buff->msg.data.len) { > tst_res(TFAIL, "Received the wrong message data length"); > > ... Ah, good, sorry I missed this. Let's go with the release then!
diff --git a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c index 5c84957b3..b0d945a11 100644 --- a/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c +++ b/testcases/kernel/syscalls/ipc/msgstress/msgstress01.c @@ -131,7 +131,7 @@ static void reader(const int id, const int pos) return; } - for (int i = 0; i < size; i++) { + for (int i = 0; i < msg_recv.data.len; i++) { if (msg_recv.data.pbytes[i] != buff->msg.data.pbytes[i]) { tst_res(TFAIL, "Received wrong data at index %d: %x != %x", i, msg_recv.data.pbytes[i],
The size returned from recvmsg() is the size of the payload but the payload is defined as: struct { char len; char pbytes[99]; } data; So the lenght of the pbytes is actually one byte shorter than the size and we access one byte after the array in the comparsion. Better fix for this would be removal of the len from the data payload but since we are close to the release lets do the minimal fix now and do the cleanup after the release. Signed-off-by: Cyril Hrubis <chrubis@suse.cz> --- testcases/kernel/syscalls/ipc/msgstress/msgstress01.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)