[LTP] [PATCH 0/3] Various fixes for out-of-bound uaccess

[LTP] [PATCH 0/3] Various fixes for out-of-bound uaccess

[Kevin Brodsky]

Hi,

This series addresses various situations where syscalls tests cause the
kernel to access arbitrary data, beyond the bounds of the object that
the test meant to specify.

Patch 1 and 2 are straightforward fixes for tests that don't pass
syscall arguments correctly.

Patch 3 is a little subtler, in that the argument is appropriately
passed, but the kernel ends up accessing arbitrary data due to the
referenced buffer being too small.

These issues were caught while running the syscalls suite on
Morello [1], using the pure-capability ABI [2]. Thanks to the capability
representation, pointers carry bounds that match the object they refer
to. The kernel accesses user memory via such capabilities, and as a
result a syscall will fail (-EFAULT) if any uaccess goes out of bounds.

A CI run can be found here [3].

Cheers,
Kevin

[1] https://www.morello-project.org/
[2] https://git.morello-project.org/morello/kernel/linux/-/wikis/Morello-pure-capability-kernel-user-Linux-ABI-specification
[3] https://github.com/kevin-brodsky-arm/ltp/actions/runs/6610988117

Kevin Brodsky (3):
  syscalls/{,f}setxattr: Fix passing of value pointer
  syscalls/msgctl06: Pass an appropriate struct to msgsnd()
  Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

 include/old/usctest.h                             | 12 ++----------
 testcases/kernel/syscalls/fsetxattr/fsetxattr01.c |  2 +-
 testcases/kernel/syscalls/ipc/msgctl/msgctl06.c   |  6 +++++-
 testcases/kernel/syscalls/rename/rename10.c       |  2 +-
 testcases/kernel/syscalls/setxattr/setxattr01.c   |  2 +-
 5 files changed, 10 insertions(+), 14 deletions(-)

-- 
2.38.1


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

[LTP] [PATCH 1/3] syscalls/{, f}setxattr: Fix passing of value pointer

[Kevin Brodsky]

tc[i].value is a pointer to the actual value pointer, it therefore
needs to be dereferenced before passing it down to {f,}setxattr()
(which is already done as expected in the TEST() line below).

This brokenness went unnoticed as the initial value (garbage before
this patch) is never read back - this call is only used to create
the key.

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 testcases/kernel/syscalls/fsetxattr/fsetxattr01.c | 2 +-
 testcases/kernel/syscalls/setxattr/setxattr01.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
index d799e477f49c..b65b27bdf9ac 100644
--- a/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
+++ b/testcases/kernel/syscalls/fsetxattr/fsetxattr01.c
@@ -140,7 +140,7 @@ static void verify_fsetxattr(unsigned int i)
 {
 	/* some tests might require existing keys for each iteration */
 	if (tc[i].keyneeded) {
-		SAFE_FSETXATTR(fd, tc[i].key, tc[i].value, tc[i].size,
+		SAFE_FSETXATTR(fd, tc[i].key, *tc[i].value, tc[i].size,
 				XATTR_CREATE);
 	}
 
diff --git a/testcases/kernel/syscalls/setxattr/setxattr01.c b/testcases/kernel/syscalls/setxattr/setxattr01.c
index 8cd2821d0c6c..31f41369a608 100644
--- a/testcases/kernel/syscalls/setxattr/setxattr01.c
+++ b/testcases/kernel/syscalls/setxattr/setxattr01.c
@@ -137,7 +137,7 @@ static void verify_setxattr(unsigned int i)
 {
 	/* some tests might require existing keys for each iteration */
 	if (tc[i].keyneeded) {
-		SAFE_SETXATTR(FNAME, tc[i].key, tc[i].value, tc[i].size,
+		SAFE_SETXATTR(FNAME, tc[i].key, *tc[i].value, tc[i].size,
 				XATTR_CREATE);
 	}
 
-- 
2.38.1


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

[LTP] [PATCH 2/3] syscalls/msgctl06: Pass an appropriate struct to msgsnd()

[Kevin Brodsky]

msgsnd() expects a pointer to a struct as second argument. If a
pointer to a short buffer is provided instead, both the type and
message read by the kernel will be garbage. This went unnoticed as
the sent message is never read back in this test.

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 testcases/kernel/syscalls/ipc/msgctl/msgctl06.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/syscalls/ipc/msgctl/msgctl06.c b/testcases/kernel/syscalls/ipc/msgctl/msgctl06.c
index 6f54763833ed..c1264b71e0e4 100644
--- a/testcases/kernel/syscalls/ipc/msgctl/msgctl06.c
+++ b/testcases/kernel/syscalls/ipc/msgctl/msgctl06.c
@@ -139,12 +139,16 @@ static void verify_msgctl(unsigned int n)
 static void setup(void)
 {
 	struct msqid_ds temp_buf;
+	struct buf {
+		long type;
+		char text[5];
+	} msgbuf = {MSGTYPE, "abcd"};
 	ltpuser = SAFE_GETPWNAM("nobody");
 	nobody_uid = ltpuser->pw_uid;
 	root_uid = 0;
 
 	msg_id = SAFE_MSGGET(IPC_PRIVATE, IPC_CREAT | MSG_RW);
-	SAFE_MSGSND(msg_id, "abcd", 4, 0);
+	SAFE_MSGSND(msg_id, &msgbuf, sizeof(msgbuf.text), 0);
 
 	TEST(msgctl(msg_id, MSG_STAT_ANY, &temp_buf));
 	if (TST_RET == -1) {
-- 
2.38.1


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

[LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Kevin Brodsky]

A number of tests check that syscalls manipulating paths return
-ENAMETOOLONG when the specified path is longer than allowed. There
are actually two ways this error can be triggered:

1. If the given string is longer than PATH_MAX, i.e. 4096 as far as
   the kernel is concerned, then the getname() helper will fail and
   the kernel will return -ENAMETOOLONG right away.

2. If the string fits in PATH_MAX, but the filesystem rejects the
   path name, for instance because one of its components is longer
   than the support file name length (e.g. 255 for ext4).

At the moment, we always fail because of 2. since we set PATH_MAX to
1024 in LTP, and in fact we sometimes use NAME_MAX as buffer length
instead (which is shorter). Thus what actually happens is that the
kernel overreads the provided buffer and finds some zero before
hitting the "real" PATH_MAX limit (4096).

We should clearly avoid such overreads. Having the syscalls fail
right away by hitting 1. is the safer option. To do so, we obtain
the value of PATH_MAX directly from the appropriate kernel header,
and ensure it is used where relevant. This affects at least the
following tests:

- rename10
- symlink03

Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com>
---
 include/old/usctest.h                       | 12 ++----------
 testcases/kernel/syscalls/rename/rename10.c |  2 +-
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/include/old/usctest.h b/include/old/usctest.h
index 9b9446d70a1e..2d46c4045111 100644
--- a/include/old/usctest.h
+++ b/include/old/usctest.h
@@ -34,16 +34,8 @@
 #ifndef __USCTEST_H__
 #define __USCTEST_H__
 
-/*
- * Ensure that PATH_MAX is defined
- */
-#ifndef PATH_MAX
-#ifdef MAXPATHLEN
-#define PATH_MAX  MAXPATHLEN
-#else
-#define PATH_MAX  1024
-#endif
-#endif
+/* For PATH_MAX */
+#include <linux/limits.h>
 
 /***********************************************************************
  * The following globals are defined in parse_opts.c but must be
diff --git a/testcases/kernel/syscalls/rename/rename10.c b/testcases/kernel/syscalls/rename/rename10.c
index 444f65366576..3eab4be0f913 100644
--- a/testcases/kernel/syscalls/rename/rename10.c
+++ b/testcases/kernel/syscalls/rename/rename10.c
@@ -18,7 +18,7 @@
 #define MNT_POINT "mntpoint"
 #define TEMP_FILE "tmpfile"
 
-static char long_path[NAME_MAX + 1] = {[0 ... NAME_MAX] = 'a'};
+static char long_path[PATH_MAX + 1] = {[0 ... PATH_MAX] = 'a'};
 
 static void setup(void)
 {
-- 
2.38.1


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

Re: [LTP] [PATCH 0/3] Various fixes for out-of-bound uaccess

[Cyril Hrubis]

Hi!
Patchset pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Cyril Hrubis]

Hi!
> A number of tests check that syscalls manipulating paths return
> -ENAMETOOLONG when the specified path is longer than allowed. There
> are actually two ways this error can be triggered:
> 
> 1. If the given string is longer than PATH_MAX, i.e. 4096 as far as
>    the kernel is concerned, then the getname() helper will fail and
>    the kernel will return -ENAMETOOLONG right away.
> 
> 2. If the string fits in PATH_MAX, but the filesystem rejects the
>    path name, for instance because one of its components is longer
>    than the support file name length (e.g. 255 for ext4).

Ideally we should have at least one test that would hit the 1. as well...

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Kevin Brodsky]

On 23/10/2023 16:40, Cyril Hrubis wrote:
> Hi!
>> A number of tests check that syscalls manipulating paths return
>> -ENAMETOOLONG when the specified path is longer than allowed. There
>> are actually two ways this error can be triggered:
>>
>> 1. If the given string is longer than PATH_MAX, i.e. 4096 as far as
>>    the kernel is concerned, then the getname() helper will fail and
>>    the kernel will return -ENAMETOOLONG right away.
>>
>> 2. If the string fits in PATH_MAX, but the filesystem rejects the
>>    path name, for instance because one of its components is longer
>>    than the support file name length (e.g. 255 for ext4).
> Ideally we should have at least one test that would hit the 1. as well...

This is what patch 3 is meant to achieve: instead of hitting 2. we now
systematically hit 1.

Thanks for merging this series (so quickly)!

Kevin

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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Cyril Hrubis]

Hi!
> >> A number of tests check that syscalls manipulating paths return
> >> -ENAMETOOLONG when the specified path is longer than allowed. There
> >> are actually two ways this error can be triggered:
> >>
> >> 1. If the given string is longer than PATH_MAX, i.e. 4096 as far as
> >>    the kernel is concerned, then the getname() helper will fail and
> >>    the kernel will return -ENAMETOOLONG right away.
> >>
> >> 2. If the string fits in PATH_MAX, but the filesystem rejects the
> >>    path name, for instance because one of its components is longer
> >>    than the support file name length (e.g. 255 for ext4).
> > Ideally we should have at least one test that would hit the 1. as well...
> 
> This is what patch 3 is meant to achieve: instead of hitting 2. we now
> systematically hit 1.

Sigh, I meant 2. I guess that we would have to loop over filesystems
(easily done with .all_filesystems = 1) and pass very long filename. Or
do we have such test already?

Looking at our tests, the rename10.c is actually one of two tests that
sets .all_fileystems and checks for ENAMETOOLONG. Looking at the
filesystem limits, all seems to have limits that are <= 255 characters,
the only problem is a definition of character. For utf8 character 255
characters are around 1021 (including nul terminator). So I suppose that
if we pass another buffer that is PATH_MAX in length and has PATH_MAX-1
characters we should consistenly hit 2. Or do I miss something?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Kevin Brodsky]

On 23/10/2023 17:06, Cyril Hrubis wrote:
> Hi!
>>>> A number of tests check that syscalls manipulating paths return
>>>> -ENAMETOOLONG when the specified path is longer than allowed. There
>>>> are actually two ways this error can be triggered:
>>>>
>>>> 1. If the given string is longer than PATH_MAX, i.e. 4096 as far as
>>>>    the kernel is concerned, then the getname() helper will fail and
>>>>    the kernel will return -ENAMETOOLONG right away.
>>>>
>>>> 2. If the string fits in PATH_MAX, but the filesystem rejects the
>>>>    path name, for instance because one of its components is longer
>>>>    than the support file name length (e.g. 255 for ext4).
>>> Ideally we should have at least one test that would hit the 1. as well...
>> This is what patch 3 is meant to achieve: instead of hitting 2. we now
>> systematically hit 1.
> Sigh, I meant 2. I guess that we would have to loop over filesystems
> (easily done with .all_filesystems = 1) and pass very long filename. Or
> do we have such test already?
>
> Looking at our tests, the rename10.c is actually one of two tests that
> sets .all_fileystems and checks for ENAMETOOLONG. Looking at the
> filesystem limits, all seems to have limits that are <= 255 characters,
> the only problem is a definition of character. For utf8 character 255
> characters are around 1021 (including nul terminator). So I suppose that
> if we pass another buffer that is PATH_MAX in length and has PATH_MAX-1
> characters we should consistenly hit 2. Or do I miss something?

This is a good point, I didn't think about it this way. Your idea seems
sensible. With this patch we always hit 1. as we specify a string that
is longer than PATH_MAX. We could instead hit 2. without out-of-bound
access by specifying a string that is at most PATH_MAX in length
(including the null terminator), and at least the filesystem character
limit. Maybe something like the diff below (just tested it, that works
fine).

Kevin

-----8<-----

--- a/testcases/kernel/syscalls/rename/rename10.c
+++ b/testcases/kernel/syscalls/rename/rename10.c
@@ -18,7 +18,7 @@
 #define MNT_POINT "mntpoint"
 #define TEMP_FILE "tmpfile"
 
-static char long_path[PATH_MAX + 1] = {[0 ... PATH_MAX] = 'a'};
+static char long_path[PATH_MAX] = {[0 ... PATH_MAX - 2] = 'a',
[PATH_MAX - 1] = '\0'};
 
 static void setup(void)
 {


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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Cyril Hrubis]

Hi!
> > Sigh, I meant 2. I guess that we would have to loop over filesystems
> > (easily done with .all_filesystems = 1) and pass very long filename. Or
> > do we have such test already?
> >
> > Looking at our tests, the rename10.c is actually one of two tests that
> > sets .all_fileystems and checks for ENAMETOOLONG. Looking at the
> > filesystem limits, all seems to have limits that are <= 255 characters,
> > the only problem is a definition of character. For utf8 character 255
> > characters are around 1021 (including nul terminator). So I suppose that
> > if we pass another buffer that is PATH_MAX in length and has PATH_MAX-1
> > characters we should consistenly hit 2. Or do I miss something?
> 
> This is a good point, I didn't think about it this way. Your idea seems
> sensible. With this patch we always hit 1. as we specify a string that
> is longer than PATH_MAX. We could instead hit 2. without out-of-bound
> access by specifying a string that is at most PATH_MAX in length
> (including the null terminator), and at least the filesystem character
> limit. Maybe something like the diff below (just tested it, that works
> fine).

Can we actually have two long paths in the test and test both? That
should have the best test coverage.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH 3/3] Provide a PATH_MAX-long buffer when expecting ENAMETOOLONG

[Kevin Brodsky]

On 24/10/2023 11:36, Cyril Hrubis wrote:
> Hi!
>>> Sigh, I meant 2. I guess that we would have to loop over filesystems
>>> (easily done with .all_filesystems = 1) and pass very long filename. Or
>>> do we have such test already?
>>>
>>> Looking at our tests, the rename10.c is actually one of two tests that
>>> sets .all_fileystems and checks for ENAMETOOLONG. Looking at the
>>> filesystem limits, all seems to have limits that are <= 255 characters,
>>> the only problem is a definition of character. For utf8 character 255
>>> characters are around 1021 (including nul terminator). So I suppose that
>>> if we pass another buffer that is PATH_MAX in length and has PATH_MAX-1
>>> characters we should consistenly hit 2. Or do I miss something?
>> This is a good point, I didn't think about it this way. Your idea seems
>> sensible. With this patch we always hit 1. as we specify a string that
>> is longer than PATH_MAX. We could instead hit 2. without out-of-bound
>> access by specifying a string that is at most PATH_MAX in length
>> (including the null terminator), and at least the filesystem character
>> limit. Maybe something like the diff below (just tested it, that works
>> fine).
> Can we actually have two long paths in the test and test both? That
> should have the best test coverage.

Certainly, that should be easy enough. I can post a follow-up patch to
that effect.

Kevin

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