| Message ID | 20221027140954.4094-1-akumar@suse.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | setfsuid02: using -1 as invalid fsuid for setfsuid() | expand |
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,
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
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.
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
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?
> 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
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.
> 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
> 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
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).
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
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
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);
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(-)