[LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[Jan Stancek]

The manpage doesn't say that 'smbuf' needs to be valid
for entire range of 'bufsize'.

This check relies on access_ok() check, which can be skipped
on some arches/configs, for example on s390x with
CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
  statmount07.c:117: TFAIL: invalid buffer size succeeded

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/statmount/statmount07.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/testcases/kernel/syscalls/statmount/statmount07.c b/testcases/kernel/syscalls/statmount/statmount07.c
index 0cc83429872f..334c287b791c 100644
--- a/testcases/kernel/syscalls/statmount/statmount07.c
+++ b/testcases/kernel/syscalls/statmount/statmount07.c
@@ -88,15 +88,6 @@ struct tcase {
 		&buff_size,
 		&st_mount
 	},
-	{
-		EFAULT,
-		"invalid buffer size",
-		&mnt_id,
-		0,
-		0,
-		&buff_size_invalid,
-		&st_mount
-	},
 	{
 		EFAULT,
 		"invalid buffer pointer",
-- 
2.43.0


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

Re: [LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[Cyril Hrubis]

Hi!
> The manpage doesn't say that 'smbuf' needs to be valid
> for entire range of 'bufsize'.
> 
> This check relies on access_ok() check, which can be skipped
> on some arches/configs, for example on s390x with
> CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
>   statmount07.c:117: TFAIL: invalid buffer size succeeded

And does it fail later on in the copy_to_user() if the buffer is
physically not accesible?

We may add a test that would look like:

| page mapped rw | page mapped read only |
             ^
	     buf pointer starts here

What do you think?

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[Jan Stancek]

On Tue, Oct 15, 2024 at 11:52 AM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > The manpage doesn't say that 'smbuf' needs to be valid
> > for entire range of 'bufsize'.
> >
> > This check relies on access_ok() check, which can be skipped
> > on some arches/configs, for example on s390x with
> > CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
> >   statmount07.c:117: TFAIL: invalid buffer size succeeded
>
> And does it fail later on in the copy_to_user() if the buffer is
> physically not accesible?
>
> We may add a test that would look like:
>
> | page mapped rw | page mapped read only |
>              ^
>              buf pointer starts here
>
> What do you think?

we can add that, it would be variation of "invalid buffer pointer" test

>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

Re: [LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[Cyril Hrubis]

Hi!
> > > This check relies on access_ok() check, which can be skipped
> > > on some arches/configs, for example on s390x with
> > > CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
> > >   statmount07.c:117: TFAIL: invalid buffer size succeeded
> >
> > And does it fail later on in the copy_to_user() if the buffer is
> > physically not accesible?
> >
> > We may add a test that would look like:
> >
> > | page mapped rw | page mapped read only |
> >              ^
> >              buf pointer starts here
> >
> > What do you think?
> 
> we can add that, it would be variation of "invalid buffer pointer" test

Looking at kernel __check_object_size() there seems to be a special case
for a NULL pointer in check_bogus_address(). The part that would check
if the page is writeable is done later. There are some checks in
check_heap_object() but I'm not 100% sure what these do. If I'm reading
it right it will abort the operation if the memory is not one continuous
vma, which is probably the case if we have two pages with different
access next to each other.

It may be interesting to go over the copy_to_user() code with someone
who understands mm and find a few cases to test.

-- 
Cyril Hrubis
chrubis@suse.cz

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

[LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Jan Stancek]

The manpage doesn't say that 'smbuf' needs to be valid
for entire range of 'bufsize'.

This check relies on access_ok() check, which can be skipped
on some arches/configs, for example on s390x with
CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
  statmount07.c:117: TFAIL: invalid buffer size succeeded

Change the test to use buffer that crosses partially
into PROT_NONE area.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/statmount/statmount07.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/statmount/statmount07.c b/testcases/kernel/syscalls/statmount/statmount07.c
index 0cc83429872f..58fcc20acce7 100644
--- a/testcases/kernel/syscalls/statmount/statmount07.c
+++ b/testcases/kernel/syscalls/statmount/statmount07.c
@@ -20,10 +20,10 @@
 static struct statmount *st_mount;
 static struct statmount *st_mount_null;
 static struct statmount *st_mount_small;
+static struct statmount *st_mount_bad;
 static uint64_t mnt_id;
 static uint64_t mnt_id_dont_exist = -1;
 static size_t buff_size;
-static size_t buff_size_invalid = -1;
 
 struct tcase {
 	int exp_errno;
@@ -90,12 +90,12 @@ struct tcase {
 	},
 	{
 		EFAULT,
-		"invalid buffer size",
+		"buffer crosses to PROT_NONE",
 		&mnt_id,
 		0,
 		0,
-		&buff_size_invalid,
-		&st_mount
+		&buff_size,
+		&st_mount_bad
 	},
 	{
 		EFAULT,
@@ -139,6 +139,7 @@ static struct tst_test test = {
 	.bufs = (struct tst_buffers []) {
 		{&st_mount, .size = sizeof(struct statmount)},
 		{&st_mount_small, .size = sizeof(struct statmount)},
+		{&st_mount_bad, .size = 1},
 		{}
 	}
 };
-- 
2.43.0


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

Re: [LTP] [PATCH] syscalls/statmount07: drop "invalid buffer size" test

[Jan Stancek]

On Tue, Oct 15, 2024 at 1:01 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > > > This check relies on access_ok() check, which can be skipped
> > > > on some arches/configs, for example on s390x with
> > > > CONFIG_ALTERNATE_USER_ADDRESS_SPACE=y. Test then fails with:
> > > >   statmount07.c:117: TFAIL: invalid buffer size succeeded
> > >
> > > And does it fail later on in the copy_to_user() if the buffer is
> > > physically not accesible?
> > >
> > > We may add a test that would look like:
> > >
> > > | page mapped rw | page mapped read only |
> > >              ^
> > >              buf pointer starts here
> > >
> > > What do you think?
> >
> > we can add that, it would be variation of "invalid buffer pointer" test
>
> Looking at kernel __check_object_size() there seems to be a special case
> for a NULL pointer in check_bogus_address(). The part that would check
> if the page is writeable is done later. There are some checks in
> check_heap_object() but I'm not 100% sure what these do. If I'm reading
> it right it will abort the operation if the memory is not one continuous
> vma, which is probably the case if we have two pages with different
> access next to each other.

Aren't all those checks for the "from" address? We only care about "to",
when copying to user-space. I sent v2 with your suggestion to use a buffer
that crosses into PROT_NONE.

>
> It may be interesting to go over the copy_to_user() code with someone
> who understands mm and find a few cases to test.
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Cyril Hrubis]

Hi!
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/statmount/statmount07.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/statmount/statmount07.c b/testcases/kernel/syscalls/statmount/statmount07.c
> index 0cc83429872f..58fcc20acce7 100644
> --- a/testcases/kernel/syscalls/statmount/statmount07.c
> +++ b/testcases/kernel/syscalls/statmount/statmount07.c
> @@ -20,10 +20,10 @@
>  static struct statmount *st_mount;
>  static struct statmount *st_mount_null;
>  static struct statmount *st_mount_small;
> +static struct statmount *st_mount_bad;
>  static uint64_t mnt_id;
>  static uint64_t mnt_id_dont_exist = -1;
>  static size_t buff_size;
> -static size_t buff_size_invalid = -1;
>  
>  struct tcase {
>  	int exp_errno;
> @@ -90,12 +90,12 @@ struct tcase {
>  	},
>  	{
>  		EFAULT,
> -		"invalid buffer size",
> +		"buffer crosses to PROT_NONE",
>  		&mnt_id,
>  		0,
>  		0,
> -		&buff_size_invalid,
> -		&st_mount
> +		&buff_size,
> +		&st_mount_bad
>  	},
>  	{
>  		EFAULT,
> @@ -139,6 +139,7 @@ static struct tst_test test = {
>  	.bufs = (struct tst_buffers []) {
>  		{&st_mount, .size = sizeof(struct statmount)},
>  		{&st_mount_small, .size = sizeof(struct statmount)},
> +		{&st_mount_bad, .size = 1},

With this we create a mapping where PROT_NONE is before the buffer, not
after it, since guarded buffers are primarily guarding about off-by-one
at the start of the buffer. There is a canaray after after the allocated
buffer that will potentialy be rewritten, but that would be detected
only at the test exit.

If I remmeber correctly the mappings will look like:

| PROT_NONE |_ CANARY BYTES |
             ^
	     And this is our 1-byte buffer.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Jan Stancek]

On Tue, Oct 15, 2024 at 2:08 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> > ---
> >  testcases/kernel/syscalls/statmount/statmount07.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/statmount/statmount07.c b/testcases/kernel/syscalls/statmount/statmount07.c
> > index 0cc83429872f..58fcc20acce7 100644
> > --- a/testcases/kernel/syscalls/statmount/statmount07.c
> > +++ b/testcases/kernel/syscalls/statmount/statmount07.c
> > @@ -20,10 +20,10 @@
> >  static struct statmount *st_mount;
> >  static struct statmount *st_mount_null;
> >  static struct statmount *st_mount_small;
> > +static struct statmount *st_mount_bad;
> >  static uint64_t mnt_id;
> >  static uint64_t mnt_id_dont_exist = -1;
> >  static size_t buff_size;
> > -static size_t buff_size_invalid = -1;
> >
> >  struct tcase {
> >       int exp_errno;
> > @@ -90,12 +90,12 @@ struct tcase {
> >       },
> >       {
> >               EFAULT,
> > -             "invalid buffer size",
> > +             "buffer crosses to PROT_NONE",
> >               &mnt_id,
> >               0,
> >               0,
> > -             &buff_size_invalid,
> > -             &st_mount
> > +             &buff_size,
> > +             &st_mount_bad
> >       },
> >       {
> >               EFAULT,
> > @@ -139,6 +139,7 @@ static struct tst_test test = {
> >       .bufs = (struct tst_buffers []) {
> >               {&st_mount, .size = sizeof(struct statmount)},
> >               {&st_mount_small, .size = sizeof(struct statmount)},
> > +             {&st_mount_bad, .size = 1},
>
> With this we create a mapping where PROT_NONE is before the buffer, not
> after it

Are you sure?

17019 08:32:23 write(2, "tst_buffers.c:57: \33[1;34mTINFO: "..., 66) = 66
17019 08:32:23 mmap(NULL, 8192, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3ff94f75000
17019 08:32:23 mprotect(0x3ff94f76000, 4096, PROT_NONE) = 0

st_mount_bad: 0x3ff94f75fff
(/proc/self/maps)
...
3ff94f2e000-3ff94f30000 rw-p 0002e000 fd:03 67110911
  /usr/lib/ld64.so.1
3ff94f75000-3ff94f76000 rw-p 00000000 00:00 0
3ff94f76000-3ff94f77000 ---p 00000000 00:00 0
3ff94f77000-3ff94f7b000 rw-p 00000000 00:00 0
3fffba5a000-3fffba7b000 rw-p 00000000 00:00 0                            [stack]
3fffba9f000-3fffbaa1000 r--p 00000000 00:00 0                            [vvar]
3fffbaa1000-3fffbaa3000 r-xp 00000000 00:00 0                            [vdso]

>, since guarded buffers are primarily guarding about off-by-one
> at the start of the buffer.

I'd expect going over end of buffer to be a lot more common.

> There is a canaray after after the allocated
> buffer that will potentialy be rewritten, but that would be detected
> only at the test exit.
>
> If I remmeber correctly the mappings will look like:
>
> | PROT_NONE |_ CANARY BYTES |
>              ^
>              And this is our 1-byte buffer.





>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Cyril Hrubis]

Hi!
> Are you sure?
> 
> 17019 08:32:23 write(2, "tst_buffers.c:57: \33[1;34mTINFO: "..., 66) = 66
> 17019 08:32:23 mmap(NULL, 8192, PROT_READ|PROT_WRITE,
> MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3ff94f75000
> 17019 08:32:23 mprotect(0x3ff94f76000, 4096, PROT_NONE) = 0
> 
> st_mount_bad: 0x3ff94f75fff
> (/proc/self/maps)
> ...
> 3ff94f2e000-3ff94f30000 rw-p 0002e000 fd:03 67110911
>   /usr/lib/ld64.so.1
> 3ff94f75000-3ff94f76000 rw-p 00000000 00:00 0
> 3ff94f76000-3ff94f77000 ---p 00000000 00:00 0
> 3ff94f77000-3ff94f7b000 rw-p 00000000 00:00 0
> 3fffba5a000-3fffba7b000 rw-p 00000000 00:00 0                            [stack]
> 3fffba9f000-3fffbaa1000 r--p 00000000 00:00 0                            [vvar]
> 3fffbaa1000-3fffbaa3000 r-xp 00000000 00:00 0                            [vdso]
> 
> >, since guarded buffers are primarily guarding about off-by-one
> > at the start of the buffer.
> 
> I'd expect going over end of buffer to be a lot more common.

Sigh, for some reason I had the case with PROT_NONE before the buffer
stored in my memory, maybe that was one of the versions the patchset
went through. Sorry for misleading reply.

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Cyril Hrubis]

Hi!
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Jan Stancek]

On Tue, Oct 15, 2024 at 3:44 PM Cyril Hrubis <chrubis@suse.cz> wrote:
>
> Hi!
> > Are you sure?
> >
> > 17019 08:32:23 write(2, "tst_buffers.c:57: \33[1;34mTINFO: "..., 66) = 66
> > 17019 08:32:23 mmap(NULL, 8192, PROT_READ|PROT_WRITE,
> > MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x3ff94f75000
> > 17019 08:32:23 mprotect(0x3ff94f76000, 4096, PROT_NONE) = 0
> >
> > st_mount_bad: 0x3ff94f75fff
> > (/proc/self/maps)
> > ...
> > 3ff94f2e000-3ff94f30000 rw-p 0002e000 fd:03 67110911
> >   /usr/lib/ld64.so.1
> > 3ff94f75000-3ff94f76000 rw-p 00000000 00:00 0
> > 3ff94f76000-3ff94f77000 ---p 00000000 00:00 0
> > 3ff94f77000-3ff94f7b000 rw-p 00000000 00:00 0
> > 3fffba5a000-3fffba7b000 rw-p 00000000 00:00 0                            [stack]
> > 3fffba9f000-3fffbaa1000 r--p 00000000 00:00 0                            [vvar]
> > 3fffbaa1000-3fffbaa3000 r-xp 00000000 00:00 0                            [vdso]
> >
> > >, since guarded buffers are primarily guarding about off-by-one
> > > at the start of the buffer.
> >
> > I'd expect going over end of buffer to be a lot more common.
>
> Sigh, for some reason I had the case with PROT_NONE before the buffer
> stored in my memory, maybe that was one of the versions the patchset
> went through. Sorry for misleading reply.

No worries, thanks for review.

>
> --
> Cyril Hrubis
> chrubis@suse.cz
>


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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Petr Vorel]

Hi Jan,

I dared to merge your patch. Thank for the fix!

Kind regards,
Petr

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

Re: [LTP] [PATCH v2] syscalls/statmount07: change "invalid buffer size" test

[Jan Stancek]

On Fri, Oct 18, 2024 at 12:23 PM Petr Vorel <pvorel@suse.cz> wrote:
>
> Hi Jan,
>
> I dared to merge your patch. Thank for the fix!

Thanks, I got sidetracked and it slipped my mind.

>
> Kind regards,
> Petr
>


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