* [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() @ 2022-10-27 14:09 Avinesh Kumar 2022-10-31 11:37 ` Petr Vorel 2022-10-31 13:01 ` Richard Palethorpe 0 siblings, 2 replies; 13+ messages in thread From: Avinesh Kumar @ 2022-10-27 14:09 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-27 14:09 [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() Avinesh Kumar @ 2022-10-31 11:37 ` Petr Vorel 2022-10-31 13:36 ` Martin Doucha 2022-10-31 13:01 ` Richard Palethorpe 1 sibling, 1 reply; 13+ messages in thread From: Petr Vorel @ 2022-10-31 11:37 UTC (permalink / raw) To: Avinesh Kumar; +Cc: ltp 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 11:37 ` Petr Vorel @ 2022-10-31 13:36 ` Martin Doucha 2022-10-31 13:50 ` Petr Vorel 0 siblings, 1 reply; 13+ messages in thread From: Martin Doucha @ 2022-10-31 13:36 UTC (permalink / raw) To: Petr Vorel, Avinesh Kumar; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 13:36 ` Martin Doucha @ 2022-10-31 13:50 ` Petr Vorel 2022-10-31 14:00 ` Martin Doucha 0 siblings, 1 reply; 13+ messages in thread From: Petr Vorel @ 2022-10-31 13:50 UTC (permalink / raw) To: Martin Doucha; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 13:50 ` Petr Vorel @ 2022-10-31 14:00 ` Martin Doucha 2022-10-31 14:56 ` Petr Vorel 0 siblings, 1 reply; 13+ messages in thread From: Martin Doucha @ 2022-10-31 14:00 UTC (permalink / raw) To: Petr Vorel; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 14:00 ` Martin Doucha @ 2022-10-31 14:56 ` Petr Vorel 2022-10-31 17:23 ` Martin Doucha 0 siblings, 1 reply; 13+ messages in thread From: Petr Vorel @ 2022-10-31 14:56 UTC (permalink / raw) To: Martin Doucha; +Cc: ltp > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 14:56 ` Petr Vorel @ 2022-10-31 17:23 ` Martin Doucha 2022-10-31 21:39 ` Petr Vorel 0 siblings, 1 reply; 13+ messages in thread From: Martin Doucha @ 2022-10-31 17:23 UTC (permalink / raw) To: Petr Vorel; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 17:23 ` Martin Doucha @ 2022-10-31 21:39 ` Petr Vorel 2022-11-02 7:40 ` Avinesh Kumar 0 siblings, 1 reply; 13+ messages in thread From: Petr Vorel @ 2022-10-31 21:39 UTC (permalink / raw) To: Martin Doucha; +Cc: ltp > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 21:39 ` Petr Vorel @ 2022-11-02 7:40 ` Avinesh Kumar 0 siblings, 0 replies; 13+ messages in thread From: Avinesh Kumar @ 2022-11-02 7:40 UTC (permalink / raw) To: Martin Doucha, Petr Vorel; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-27 14:09 [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() Avinesh Kumar 2022-10-31 11:37 ` Petr Vorel @ 2022-10-31 13:01 ` Richard Palethorpe 2022-10-31 21:40 ` Petr Vorel 1 sibling, 1 reply; 13+ messages in thread From: Richard Palethorpe @ 2022-10-31 13:01 UTC (permalink / raw) To: Avinesh Kumar; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 13:01 ` Richard Palethorpe @ 2022-10-31 21:40 ` Petr Vorel 2022-11-01 9:03 ` Richard Palethorpe 0 siblings, 1 reply; 13+ messages in thread From: Petr Vorel @ 2022-10-31 21:40 UTC (permalink / raw) To: Richard Palethorpe; +Cc: ltp > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-10-31 21:40 ` Petr Vorel @ 2022-11-01 9:03 ` Richard Palethorpe 2022-11-02 7:52 ` Avinesh Kumar 0 siblings, 1 reply; 13+ messages in thread From: Richard Palethorpe @ 2022-11-01 9:03 UTC (permalink / raw) To: Petr Vorel; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() 2022-11-01 9:03 ` Richard Palethorpe @ 2022-11-02 7:52 ` Avinesh Kumar 0 siblings, 0 replies; 13+ messages in thread From: Avinesh Kumar @ 2022-11-02 7:52 UTC (permalink / raw) To: rpalethorpe; +Cc: ltp 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-11-02 7:52 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-27 14:09 [LTP] [PATCH] setfsuid02: using -1 as invalid fsuid for setfsuid() Avinesh Kumar 2022-10-31 11:37 ` Petr Vorel 2022-10-31 13:36 ` Martin Doucha 2022-10-31 13:50 ` Petr Vorel 2022-10-31 14:00 ` Martin Doucha 2022-10-31 14:56 ` Petr Vorel 2022-10-31 17:23 ` Martin Doucha 2022-10-31 21:39 ` Petr Vorel 2022-11-02 7:40 ` Avinesh Kumar 2022-10-31 13:01 ` Richard Palethorpe 2022-10-31 21:40 ` Petr Vorel 2022-11-01 9:03 ` Richard Palethorpe 2022-11-02 7:52 ` Avinesh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).