[LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Cyril Hrubis]

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],
-- 
2.43.2


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Martin Doucha]

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],

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Cyril Hrubis]

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,

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Petr Vorel]

> 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,

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Martin Doucha]

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.

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Cyril Hrubis]

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...

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Martin Doucha]

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!

-- 
Martin Doucha   mdoucha@suse.cz
SW Quality Engineer
SUSE LINUX, s.r.o.
CORSO IIa
Krizikova 148/34
186 00 Prague 8
Czech Republic


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Petr Vorel]

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

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] syscalls/msgstress01: Fix off by one in array access

[Petr Vorel]

Hi Cyril,

thanks for a quick fix, merged!

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp