[LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Avinesh Kumar]

a uid which does not have an entry in the /etc/passwd
file is not really an invalid fsuid for setfsuid(), so changing
the test to use -1 as an invalid fsuid.
And second setfsuid(-1) call is to verify that preceding call has
actually failed and there is no change in the fsuid.

Signed-off-by: Avinesh Kumar <akumar@suse.de>
---
 testcases/kernel/syscalls/setfsuid/setfsuid02.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/setfsuid/setfsuid02.c b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
index 850f17834..f5aa1c004 100644
--- a/testcases/kernel/syscalls/setfsuid/setfsuid02.c
+++ b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
@@ -21,9 +21,7 @@ static void run(void)
 	uid_t invalid_uid, current_uid;
 
 	current_uid = geteuid();
-	invalid_uid = 1;
-	while (getpwuid(invalid_uid))
-		invalid_uid++;
+	invalid_uid = -1;
 
 	UID16_CHECK(invalid_uid, setfsuid);
 
-- 
2.38.0


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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Petr Vorel]

Hi Avinesh,

> a uid which does not have an entry in the /etc/passwd
> file is not really an invalid fsuid for setfsuid(), so changing
> the test to use -1 as an invalid fsuid.
> And second setfsuid(-1) call is to verify that preceding call has
> actually failed and there is no change in the fsuid.

Here was supposed to be
Fixes: 85f0b8478 ("setfsuid02: Rewrite using new LTP API")

as the problem was introduced in your rewrite, right?

> diff --git a/testcases/kernel/syscalls/setfsuid/setfsuid02.c b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> index 850f17834..f5aa1c004 100644
> --- a/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> +++ b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> @@ -21,9 +21,7 @@ static void run(void)
>  	uid_t invalid_uid, current_uid;

>  	current_uid = geteuid();
> -	invalid_uid = 1;
> -	while (getpwuid(invalid_uid))
> -		invalid_uid++;
> +	invalid_uid = -1;

IMHO it should be casted

invalid_uid = (uid_t)-1;

as the code in shadow-utils:
https://github.com/shadow-maint/shadow/blob/eaebea55a495a56317ed85e959b3599f73c6bdf2/src/newusers.c#L342-L345

This does not work on 16-bit version, because uid_t is unsigned int,
-1 overflows the allowed value when we do check:

UID16_CHECK(invalid_uid, setfsuid);

setfsuid02.c:26: TBROK: uid -1 of invalid_uid is too large for testing 16-bit version of setfsuid()

It also does not make sense to check invalid_uid, it should have been
current_uid in 85f0b8478 (my bad not catching this):

UID16_CHECK(current_uid, setfsuid);

Please, always try to test 16-bit variant (most of us does not bother to test
32-bit compatibility, but at least to see TCONF is worth).

If you agree, I merge it with changes below.

Kind regards,
Petr

diff --git testcases/kernel/syscalls/setfsuid/setfsuid02.c testcases/kernel/syscalls/setfsuid/setfsuid02.c
index f5aa1c004..92e1979fa 100644
--- testcases/kernel/syscalls/setfsuid/setfsuid02.c
+++ testcases/kernel/syscalls/setfsuid/setfsuid02.c
@@ -21,9 +21,9 @@ static void run(void)
 	uid_t invalid_uid, current_uid;
 
 	current_uid = geteuid();
-	invalid_uid = -1;
+	invalid_uid = (uid_t)-1;
 
-	UID16_CHECK(invalid_uid, setfsuid);
+	UID16_CHECK(current_uid, setfsuid);
 
 	TST_EXP_VAL_SILENT(SETFSUID(invalid_uid), current_uid);
 	TST_EXP_VAL(SETFSUID(invalid_uid), current_uid,

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Richard Palethorpe]

Hello,

Avinesh Kumar <akumar@suse.de> writes:

> a uid which does not have an entry in the /etc/passwd
> file is not really an invalid fsuid for setfsuid(), so changing
> the test to use -1 as an invalid fsuid.
> And second setfsuid(-1) call is to verify that preceding call has
> actually failed and there is no change in the fsuid.

I think the original test is flawed and testing what using -1 does is
not very interesting as the kernel uses standard boilerplate to deal
with this.

AFAICT we don't test what happens if a non-root user tries to set the
fsuid to a uid that is not the euid, ruid or saved uid or 0/-1.

Possibly that is something for a new test though.

>
> Signed-off-by: Avinesh Kumar <akumar@suse.de>
> ---
>  testcases/kernel/syscalls/setfsuid/setfsuid02.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/testcases/kernel/syscalls/setfsuid/setfsuid02.c b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> index 850f17834..f5aa1c004 100644
> --- a/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> +++ b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> @@ -21,9 +21,7 @@ static void run(void)
>  	uid_t invalid_uid, current_uid;
>  
>  	current_uid = geteuid();
> -	invalid_uid = 1;
> -	while (getpwuid(invalid_uid))
> -		invalid_uid++;
> +	invalid_uid = -1;
>  
>  	UID16_CHECK(invalid_uid, setfsuid);
>  
> -- 
> 2.38.0


-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Martin Doucha]

On 31. 10. 22 12:37, Petr Vorel wrote:
> Hi Avinesh,
> 
>> a uid which does not have an entry in the /etc/passwd
>> file is not really an invalid fsuid for setfsuid(), so changing
>> the test to use -1 as an invalid fsuid.
>> And second setfsuid(-1) call is to verify that preceding call has
>> actually failed and there is no change in the fsuid.
> 
> Here was supposed to be
> Fixes: 85f0b8478 ("setfsuid02: Rewrite using new LTP API")
> 
> as the problem was introduced in your rewrite, right?

No, the original test was already broken, it just didn't do any real 
failure checks so it always passed.

> It also does not make sense to check invalid_uid, it should have been
> current_uid in 85f0b8478 (my bad not catching this):
> 
> UID16_CHECK(current_uid, setfsuid);

No, UID16_CHECK(invalid_uid, setfsuid); is the correct test call. The 
test is supposed to verify that trying to set invalid_uid will fail, and 
the only way to verify that it failed is to call setfsuid(invalid_uid) 
again and check that it returns current_uid.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
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] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Petr Vorel]

Hi Martin, Avinesh,

> On 31. 10. 22 12:37, Petr Vorel wrote:
> > Hi Avinesh,

> > > a uid which does not have an entry in the /etc/passwd
> > > file is not really an invalid fsuid for setfsuid(), so changing
> > > the test to use -1 as an invalid fsuid.
> > > And second setfsuid(-1) call is to verify that preceding call has
> > > actually failed and there is no change in the fsuid.

> > Here was supposed to be
> > Fixes: 85f0b8478 ("setfsuid02: Rewrite using new LTP API")

> > as the problem was introduced in your rewrite, right?

> No, the original test was already broken, it just didn't do any real failure
> checks so it always passed.
Right, thanks!

> > It also does not make sense to check invalid_uid, it should have been
> > current_uid in 85f0b8478 (my bad not catching this):

> > UID16_CHECK(current_uid, setfsuid);

> No, UID16_CHECK(invalid_uid, setfsuid); is the correct test call. The test
> is supposed to verify that trying to set invalid_uid will fail, and the only
> way to verify that it failed is to call setfsuid(invalid_uid) again and
> check that it returns current_uid.
I'm somehow blind today. UID16_CHECK() in compat_tst_16.h calls
UID_SIZE_CHECK(), which just checks the uid value. I don't see setfsuid() call,
IMHO that's done in SETFSUID().

Kind regards,
Petr

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Martin Doucha]

On 31. 10. 22 14:50, Petr Vorel wrote:
>> No, UID16_CHECK(invalid_uid, setfsuid); is the correct test call. The test
>> is supposed to verify that trying to set invalid_uid will fail, and the only
>> way to verify that it failed is to call setfsuid(invalid_uid) again and
>> check that it returns current_uid.
> I'm somehow blind today. UID16_CHECK() in compat_tst_16.h calls
> UID_SIZE_CHECK(), which just checks the uid value. I don't see setfsuid() call,
> IMHO that's done in SETFSUID().

It doesn't make sense to call UID16_CHECK() on a value that we'll never 
pass to a 16bit syscall, does it?

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
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] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Petr Vorel]

> On 31. 10. 22 14:50, Petr Vorel wrote:
> > > No, UID16_CHECK(invalid_uid, setfsuid); is the correct test call. The test
> > > is supposed to verify that trying to set invalid_uid will fail, and the only
> > > way to verify that it failed is to call setfsuid(invalid_uid) again and
> > > check that it returns current_uid.
> > I'm somehow blind today. UID16_CHECK() in compat_tst_16.h calls
> > UID_SIZE_CHECK(), which just checks the uid value. I don't see setfsuid() call,
> > IMHO that's done in SETFSUID().

> It doesn't make sense to call UID16_CHECK() on a value that we'll never pass
> to a 16bit syscall, does it?
Ah, sure, I see current_uid is used for checking only. So what is your
suggestion to fix the problem on 16-bit?

Kind regards,
Petr

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Martin Doucha]

On 31. 10. 22 15:56, Petr Vorel wrote:
> Ah, sure, I see current_uid is used for checking only. So what is your
> suggestion to fix the problem on 16-bit?

I guess using UID_T (defined in the LTP compat headers) instead of uid_t 
is the solution, then -1 will be cast to the correct bitsize.

-- 
Martin Doucha   mdoucha@suse.cz
QA Engineer for Software Maintenance
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] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Petr Vorel]

> On 31. 10. 22 15:56, Petr Vorel wrote:
> > Ah, sure, I see current_uid is used for checking only. So what is your
> > suggestion to fix the problem on 16-bit?

> I guess using UID_T (defined in the LTP compat headers) instead of uid_t is
> the solution, then -1 will be cast to the correct bitsize.

Thank you, that worked.

Merged, with additional cast to long to keep compiler happy.

Kind regards,
Petr

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Petr Vorel]

> Hello,

> Avinesh Kumar <akumar@suse.de> writes:

> > a uid which does not have an entry in the /etc/passwd
> > file is not really an invalid fsuid for setfsuid(), so changing
> > the test to use -1 as an invalid fsuid.
> > And second setfsuid(-1) call is to verify that preceding call has
> > actually failed and there is no change in the fsuid.

> I think the original test is flawed and testing what using -1 does is
> not very interesting as the kernel uses standard boilerplate to deal
> with this.

> AFAICT we don't test what happens if a non-root user tries to set the
> fsuid to a uid that is not the euid, ruid or saved uid or 0/-1.

> Possibly that is something for a new test though.

Ah, sorry, I overlooked this, merged now.

Kind regards,
Petr


> > Signed-off-by: Avinesh Kumar <akumar@suse.de>
> > ---
> >  testcases/kernel/syscalls/setfsuid/setfsuid02.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)

> > diff --git a/testcases/kernel/syscalls/setfsuid/setfsuid02.c b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> > index 850f17834..f5aa1c004 100644
> > --- a/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> > +++ b/testcases/kernel/syscalls/setfsuid/setfsuid02.c
> > @@ -21,9 +21,7 @@ static void run(void)
> >  	uid_t invalid_uid, current_uid;

> >  	current_uid = geteuid();
> > -	invalid_uid = 1;
> > -	while (getpwuid(invalid_uid))
> > -		invalid_uid++;
> > +	invalid_uid = -1;

> >  	UID16_CHECK(invalid_uid, setfsuid);

> > -- 
> > 2.38.0

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Richard Palethorpe]

Hello,

Petr Vorel <pvorel@suse.cz> writes:

>> Hello,
>
>> Avinesh Kumar <akumar@suse.de> writes:
>
>> > a uid which does not have an entry in the /etc/passwd
>> > file is not really an invalid fsuid for setfsuid(), so changing
>> > the test to use -1 as an invalid fsuid.
>> > And second setfsuid(-1) call is to verify that preceding call has
>> > actually failed and there is no change in the fsuid.
>
>> I think the original test is flawed and testing what using -1 does is
>> not very interesting as the kernel uses standard boilerplate to deal
>> with this.
>
>> AFAICT we don't test what happens if a non-root user tries to set the
>> fsuid to a uid that is not the euid, ruid or saved uid or 0/-1.
>
>> Possibly that is something for a new test though.
>
> Ah, sorry, I overlooked this, merged now.

No problem, I think that is the correct thing to do. I'll leave it to
Avinesh or someone else to extend the test (or not).

-- 
Thank you,
Richard.

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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Avinesh Kumar]

Hi Petr, Martin,

Thank you for the discussion and fixing the patch for 16-bit.
I am making a note to always test 16-bit binaries also.

On Tuesday, November 1, 2022 3:09:20 AM IST Petr Vorel wrote:
> > On 31. 10. 22 15:56, Petr Vorel wrote:
> > > Ah, sure, I see current_uid is used for checking only. So what is your
> > > suggestion to fix the problem on 16-bit?
> 
> > I guess using UID_T (defined in the LTP compat headers) instead of uid_t is
> > the solution, then -1 will be cast to the correct bitsize.
> 
> Thank you, that worked.
> 
> Merged, with additional cast to long to keep compiler happy.
> 
> Kind regards,
> Petr
> 

Regards,
Avinesh



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

Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid()

[Avinesh Kumar]

Hi Richie,

On Tuesday, November 1, 2022 2:33:00 PM IST Richard Palethorpe wrote:
> Hello,
> 
> Petr Vorel <pvorel@suse.cz> writes:
> 
> >> Hello,
> >
> >> Avinesh Kumar <akumar@suse.de> writes:
> >
> >> > a uid which does not have an entry in the /etc/passwd
> >> > file is not really an invalid fsuid for setfsuid(), so changing
> >> > the test to use -1 as an invalid fsuid.
> >> > And second setfsuid(-1) call is to verify that preceding call has
> >> > actually failed and there is no change in the fsuid.
> >
> >> I think the original test is flawed and testing what using -1 does is
> >> not very interesting as the kernel uses standard boilerplate to deal
> >> with this.
> >
> >> AFAICT we don't test what happens if a non-root user tries to set the
> >> fsuid to a uid that is not the euid, ruid or saved uid or 0/-1.

> >
> >> Possibly that is something for a new test though.
I'll post a new patch for this. Thank you for the suggestion.

> >
> > Ah, sorry, I overlooked this, merged now.
> 
> No problem, I think that is the correct thing to do. I'll leave it to
> Avinesh or someone else to extend the test (or not).
> 
> 

Regards,
Avinesh



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