From: Stanislav K. <sta...@or...> - 2013-11-08 08:37:57
|
Commit 705926d096f7a64c9d53efeb789288e25457d68f introduced a bug that the value for inval_user was treated as '-1' when passed to 16-bit version of setregid(). Fixed it the same way as it's done for setreuid test cases. Reported-by: Honggyu Kim <hon...@lg...> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/setregid/setregid02.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/testcases/kernel/syscalls/setregid/setregid02.c b/testcases/kernel/syscalls/setregid/setregid02.c index e67a7df..2b84b26 100644 --- a/testcases/kernel/syscalls/setregid/setregid02.c +++ b/testcases/kernel/syscalls/setregid/setregid02.c @@ -39,7 +39,7 @@ TCID_DEFINE(setregid02); static gid_t neg_one = -1; -static gid_t inval_user = (USHRT_MAX); +static gid_t inval_user = USHRT_MAX - 2; static struct passwd *ltpuser; static struct group nobody, root, bin; -- 1.7.1 |
From: <ch...@su...> - 2013-11-13 12:46:47
|
Hi! > Commit 705926d096f7a64c9d53efeb789288e25457d68f > introduced a bug that the value for inval_user was treated > as '-1' when passed to 16-bit version of setregid(). > > Fixed it the same way as it's done for setreuid test cases. Pushed, thanks. -- Cyril Hrubis ch...@su... |
From: <ch...@su...> - 2014-01-08 16:56:27
|
Hi! > Commit 705926d096f7a64c9d53efeb789288e25457d68f > introduced a bug that the value for inval_user was treated > as '-1' when passed to 16-bit version of setregid(). > > Fixed it the same way as it's done for setreuid test cases. > > Reported-by: Honggyu Kim <hon...@lg...> > Signed-off-by: Stanislav Kholmanskikh <sta...@or...> > --- > testcases/kernel/syscalls/setregid/setregid02.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/testcases/kernel/syscalls/setregid/setregid02.c b/testcases/kernel/syscalls/setregid/setregid02.c > index e67a7df..2b84b26 100644 > --- a/testcases/kernel/syscalls/setregid/setregid02.c > +++ b/testcases/kernel/syscalls/setregid/setregid02.c > @@ -39,7 +39,7 @@ TCID_DEFINE(setregid02); > > static gid_t neg_one = -1; > > -static gid_t inval_user = (USHRT_MAX); > +static gid_t inval_user = USHRT_MAX - 2; > static struct passwd *ltpuser; Unfortunately I've found that in SLES user nobody has UID precisely USHRT_MAX - 2 and the test fails :( Thinking of correct solution, we probably need some tst_get_unused_uid(). What do you think? -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2014-01-09 06:10:26
|
On 01/08/2014 08:56 PM, ch...@su... wrote: > Hi! >> Commit 705926d096f7a64c9d53efeb789288e25457d68f >> introduced a bug that the value for inval_user was treated >> as '-1' when passed to 16-bit version of setregid(). >> >> Fixed it the same way as it's done for setreuid test cases. >> >> Reported-by: Honggyu Kim <hon...@lg...> >> Signed-off-by: Stanislav Kholmanskikh <sta...@or...> >> --- >> testcases/kernel/syscalls/setregid/setregid02.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/testcases/kernel/syscalls/setregid/setregid02.c b/testcases/kernel/syscalls/setregid/setregid02.c >> index e67a7df..2b84b26 100644 >> --- a/testcases/kernel/syscalls/setregid/setregid02.c >> +++ b/testcases/kernel/syscalls/setregid/setregid02.c >> @@ -39,7 +39,7 @@ TCID_DEFINE(setregid02); >> >> static gid_t neg_one = -1; >> >> -static gid_t inval_user = (USHRT_MAX); >> +static gid_t inval_user = USHRT_MAX - 2; >> static struct passwd *ltpuser; > > Unfortunately I've found that in SLES user nobody has UID precisely > USHRT_MAX - 2 and the test fails :( > > Thinking of correct solution, we probably need some > tst_get_unused_uid(). What do you think? > Hi! Ok, I will do it. |
From: <ch...@su...> - 2014-01-09 16:41:02
|
Hi! > > Unfortunately I've found that in SLES user nobody has UID precisely > > USHRT_MAX - 2 and the test fails :( > > > > Thinking of correct solution, we probably need some > > tst_get_unused_uid(). What do you think? > > > > Hi! Ok, I will do it. > Do you have some estimate when will the patch be ready? I would like to have this fix included in the release, if possible. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2014-01-10 10:09:33
|
On 01/09/2014 08:40 PM, ch...@su... wrote: > Hi! >>> Unfortunately I've found that in SLES user nobody has UID precisely >>> USHRT_MAX - 2 and the test fails :( >>> >>> Thinking of correct solution, we probably need some >>> tst_get_unused_uid(). What do you think? >>> >> >> Hi! Ok, I will do it. >> > > Do you have some estimate when will the patch be ready? I would like to > have this fix included in the release, if possible. > I'll try to do it today. In the worst case it will be the first half of next Monday (Jan 13). I'm in UTC+4. |
From: Stanislav K. <sta...@or...> - 2014-01-13 09:35:37
|
Since there is no guarantee that (USHRT_MAX - 2) uid/gid is free on the platform it would be better to use special functions: * tst_get_unused_uid * tst_get_unused_gid wrapped for 16-bit compatibility: if on the host there is no free uid/gid in between [0; 0xFFFE] the 16-bit test case should return TBROK. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- testcases/kernel/syscalls/setregid/setregid02.c | 6 +++++- testcases/kernel/syscalls/setresuid/setresuid03.c | 8 +++++++- testcases/kernel/syscalls/utils/compat_gid.h | 20 ++++++++++++++++++++ testcases/kernel/syscalls/utils/compat_uid.h | 20 ++++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/testcases/kernel/syscalls/setregid/setregid02.c b/testcases/kernel/syscalls/setregid/setregid02.c index ccbaa3e..a730a5e 100644 --- a/testcases/kernel/syscalls/setregid/setregid02.c +++ b/testcases/kernel/syscalls/setregid/setregid02.c @@ -39,7 +39,7 @@ TCID_DEFINE(setregid02); static gid_t neg_one = -1; -static gid_t inval_user = USHRT_MAX - 2; +static gid_t inval_user; static struct passwd *ltpuser; static struct group nobody, root, bin; @@ -177,6 +177,10 @@ static void setup(void) GET_GID(nobody); GET_GID(bin); + inval_user = GET_UNUSED_GID(); + if (inval_user == -1) + tst_brkm(TBROK, NULL, "No free gid found"); + TEST_PAUSE; } diff --git a/testcases/kernel/syscalls/setresuid/setresuid03.c b/testcases/kernel/syscalls/setresuid/setresuid03.c index 9bb0abd..085ed7b 100644 --- a/testcases/kernel/syscalls/setresuid/setresuid03.c +++ b/testcases/kernel/syscalls/setresuid/setresuid03.c @@ -70,10 +70,12 @@ #include <errno.h> #include <sys/wait.h> +#include <compat_16.h> + char *TCID = "setresuid03"; uid_t neg_one = -1; -uid_t inval_user = (USHRT_MAX - 2); +uid_t inval_user; /* flag to tell parent if child passed or failed. */ int flag = 0; @@ -232,6 +234,10 @@ void setup(void) bin = *(getpwnam("bin")); bin_pw_uid = bin.pw_uid; + inval_user = GET_UNUSED_UID(); + if (inval_user == -1) + tst_brkm(TBROK, NULL, "No free uid found"); + /* Pause if that option was specified * TEST_PAUSE contains the code to fork the test with the -i option. * You want to make sure you do this before you create your temporary diff --git a/testcases/kernel/syscalls/utils/compat_gid.h b/testcases/kernel/syscalls/utils/compat_gid.h index 82f8d84..1ce713b 100644 --- a/testcases/kernel/syscalls/utils/compat_gid.h +++ b/testcases/kernel/syscalls/utils/compat_gid.h @@ -48,4 +48,24 @@ GID_SIZE_CHECK(gid_t gid) #endif +/* for 16-bit syscalls testing we can only + * use gids <= 0xFFFE */ +gid_t GET_UNUSED_GID(void) +{ + gid_t r; + r = tst_get_unused_gid(); + +#ifdef TST_USE_COMPAT16_SYSCALL + if (!GID_SIZE_CHECK(r)) + return -1; + + /* kernel low2highgid() converts + * 0xFFFF to (gid_t)-1 */ + if (r == (GID_T)-1) + return -1; +#endif + + return r; +} + #endif /* __SETGID_COMPAT_16_H__ */ diff --git a/testcases/kernel/syscalls/utils/compat_uid.h b/testcases/kernel/syscalls/utils/compat_uid.h index 8200698..68896f4 100644 --- a/testcases/kernel/syscalls/utils/compat_uid.h +++ b/testcases/kernel/syscalls/utils/compat_uid.h @@ -48,4 +48,24 @@ UID_SIZE_CHECK(uid_t uid) #endif +/* for 16-bit syscalls testing we can only + * use uids <= 0xFFFE */ +uid_t GET_UNUSED_UID(void) +{ + uid_t r; + r = tst_get_unused_uid(); + +#ifdef TST_USE_COMPAT16_SYSCALL + if (!UID_SIZE_CHECK(r)) + return -1; + + /* kernel low2highuid() converts + * 0xFFFF to (uid_t)-1 */ + if (r == (UID_T)-1) + return -1; +#endif + + return r; +} + #endif /* __SETUID_COMPAT_16_H__ */ -- 1.7.1 |
From: Stanislav K. <sta...@or...> - 2014-01-13 09:35:38
|
Implemented functions which return the first unused uid and gid. Signed-off-by: Stanislav Kholmanskikh <sta...@or...> --- include/test.h | 7 ++++ lib/tst_uid_gid.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 0 deletions(-) create mode 100644 lib/tst_uid_gid.c diff --git a/include/test.h b/include/test.h index ffc1c8c..9f4cfd7 100644 --- a/include/test.h +++ b/include/test.h @@ -264,6 +264,13 @@ void tst_mkfs(void (cleanup_fn)(void), const char *dev, */ int tst_fill_file(const char *path, char pattern, size_t bs, size_t bcount); +/* lib/tst_uid_gid.c + * + * Return the first unused uid and gid + */ +uid_t tst_get_unused_uid(void); +gid_t tst_get_unused_gid(void); + #ifdef TST_USE_COMPAT16_SYSCALL #define TCID_BIT_SUFFIX "_16" #elif TST_USE_NEWER64_SYSCALL diff --git a/lib/tst_uid_gid.c b/lib/tst_uid_gid.c new file mode 100644 index 0000000..e462d45 --- /dev/null +++ b/lib/tst_uid_gid.c @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <grp.h> +#include <limits.h> +#include <pwd.h> +#include <stdlib.h> +#include <sys/types.h> +#include <unistd.h> + +uid_t tst_get_unused_uid(void) +{ + struct passwd pwd; + struct passwd *result; + char *buf; + size_t bufsize; + int s; + uid_t uid; + + bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); + if (bufsize == -1) + bufsize = 16384; + + buf = malloc(bufsize); + if (buf == NULL) + return -1; + + for (uid = 0; uid <= UINT_MAX - 1; uid++) { + s = getpwuid_r(uid, &pwd, buf, bufsize, &result); + if (result == NULL) { + if (s == 0) + return uid; + else + return -1; + } + } + + free(buf); + return -1; +} + +gid_t tst_get_unused_gid(void) +{ + struct group grp; + struct group *result; + char *buf; + size_t bufsize; + int s; + gid_t gid; + + bufsize = sysconf(_SC_GETGR_R_SIZE_MAX); + if (bufsize == -1) + bufsize = 16384; + + buf = malloc(bufsize); + if (buf == NULL) + return -1; + + for (gid = 0; gid <= UINT_MAX - 1; gid++) { + s = getgrgid_r(gid, &grp, buf, bufsize, &result); + if (result == NULL) { + if (s == 0) + return gid; + else + return -1; + } + } + + free(buf); + return -1; +} -- 1.7.1 |
From: <ch...@su...> - 2014-01-13 17:33:00
|
Hi! > @@ -0,0 +1,86 @@ > +/* > + * Copyright (c) 2013 Oracle and/or its affiliates. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it would be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write the Free Software Foundation, > + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +#include <grp.h> > +#include <limits.h> > +#include <pwd.h> > +#include <stdlib.h> > +#include <sys/types.h> > +#include <unistd.h> > + > +uid_t tst_get_unused_uid(void) > +{ > + struct passwd pwd; > + struct passwd *result; > + char *buf; > + size_t bufsize; I've changed the bufsize to long, because size_t is unsigned. Side note: I wonder why ltp has only -Wall and not -Wall -W which prints warnings when signed and unsigned types are compared... > + int s; > + uid_t uid; > + > + bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); > + if (bufsize == -1) > + bufsize = 16384; > + > + buf = malloc(bufsize); > + if (buf == NULL) > + return -1; > + > + for (uid = 0; uid <= UINT_MAX - 1; uid++) { > + s = getpwuid_r(uid, &pwd, buf, bufsize, &result); > + if (result == NULL) { I've added a free(buf) here otherwise we will leak memory. > + if (s == 0) > + return uid; > + else > + return -1; > + } > + } > + > + free(buf); > + return -1; > +} And the same for the second one. -- Cyril Hrubis ch...@su... |
From: <ch...@su...> - 2014-01-13 17:39:20
|
Hi! > diff --git a/testcases/kernel/syscalls/setresuid/setresuid03.c b/testcases/kernel/syscalls/setresuid/setresuid03.c > index 9bb0abd..085ed7b 100644 > --- a/testcases/kernel/syscalls/setresuid/setresuid03.c > +++ b/testcases/kernel/syscalls/setresuid/setresuid03.c > @@ -70,10 +70,12 @@ > #include <errno.h> > #include <sys/wait.h> > > +#include <compat_16.h> > + > char *TCID = "setresuid03"; > > uid_t neg_one = -1; > -uid_t inval_user = (USHRT_MAX - 2); > +uid_t inval_user; > > /* flag to tell parent if child passed or failed. */ > int flag = 0; > @@ -232,6 +234,10 @@ void setup(void) > bin = *(getpwnam("bin")); > bin_pw_uid = bin.pw_uid; > > + inval_user = GET_UNUSED_UID(); > + if (inval_user == -1) > + tst_brkm(TBROK, NULL, "No free uid found"); > + > /* Pause if that option was specified > * TEST_PAUSE contains the code to fork the test with the -i option. > * You want to make sure you do this before you create your temporary Hmm, the setresuid03 did not fail because the EPERM check kicks in even before the check for correct uid/gid. Which should probably be fixed because there does not seem to be a setresuid test for invalid values that would actually excercise such checks, but that can be fixed after the release. Thanks for the patches, I will commit them right after packages are build and I have tested that everything is fine. -- Cyril Hrubis ch...@su... |
From: Stanislav K. <sta...@or...> - 2014-01-14 08:21:30
|
On 01/13/2014 09:38 PM, ch...@su... wrote: > Hi! >> diff --git a/testcases/kernel/syscalls/setresuid/setresuid03.c b/testcases/kernel/syscalls/setresuid/setresuid03.c >> index 9bb0abd..085ed7b 100644 >> --- a/testcases/kernel/syscalls/setresuid/setresuid03.c >> +++ b/testcases/kernel/syscalls/setresuid/setresuid03.c >> @@ -70,10 +70,12 @@ >> #include <errno.h> >> #include <sys/wait.h> >> >> +#include <compat_16.h> >> + >> char *TCID = "setresuid03"; >> >> uid_t neg_one = -1; >> -uid_t inval_user = (USHRT_MAX - 2); >> +uid_t inval_user; >> >> /* flag to tell parent if child passed or failed. */ >> int flag = 0; >> @@ -232,6 +234,10 @@ void setup(void) >> bin = *(getpwnam("bin")); >> bin_pw_uid = bin.pw_uid; >> >> + inval_user = GET_UNUSED_UID(); >> + if (inval_user == -1) >> + tst_brkm(TBROK, NULL, "No free uid found"); >> + >> /* Pause if that option was specified >> * TEST_PAUSE contains the code to fork the test with the -i option. >> * You want to make sure you do this before you create your temporary > > Hmm, the setresuid03 did not fail because the EPERM check kicks in even > before the check for correct uid/gid. Which should probably be fixed > because there does not seem to be a setresuid test for invalid values > that would actually excercise such checks, but that can be fixed after > the release. Hmm. I took a brief look at setregid() and setresuid() syscalls sources. And it seems that the only thing they care about is not "validity" or "invalidity" of uid/gid. It's the process permission (capabilities). Given enough permission we can change real, effective or saved uid values with setresuid() to arbitrary values. If we don't have such permission we can change our uids only to the one of the current real, effective or saved uid values, in the other case we will receive EPERM. Actually, man setresuid states that :-[ setresuid() may return EINVAL, but I don't think we can raise it by passing an unused uid value to setresuid() (if we have permission setresuid() returns 0 if we don't - it returns EPERM). So it's not bad that we are introducing tst_get_unused_uid, tst_get_unused_gid functions and setting inval_user in a more formal way. But It looks like passing inval_user value is the same as passing any other uid != real|effective|saved uid of the current process.... Since we have in setresuid03 a full set for nobody uid: setresuid(nobody, -1, -1) setresuid(-1, -1, nobody) setresuid(-1, nobody, -1) what do you think about adding setresuid(inval_user, -1, -1) to the test case? Just in case... Related to setregid02. Even that test_data contains EINVAL for the last two members, setregid() doesn't return EINVAL for them. It returns EPERM for all the test cases. You said that on SUSE setregid02 fails. I added a group with 65533 gid and executed setregid02 (current git version) and it passed. Could you check it again in your environment? > > Thanks for the patches, I will commit them right after packages are > build and I have tested that everything is fine. > |
From: <ch...@su...> - 2014-01-14 15:05:31
|
Hi! > > Hmm, the setresuid03 did not fail because the EPERM check kicks in even > > before the check for correct uid/gid. Which should probably be fixed > > because there does not seem to be a setresuid test for invalid values > > that would actually excercise such checks, but that can be fixed after > > the release. > > Hmm. I took a brief look at setregid() and setresuid() syscalls sources. > And it seems that the only thing they care about is not "validity" or > "invalidity" of uid/gid. It's the process permission (capabilities). > > Given enough permission we can change real, effective or saved uid > values with setresuid() to arbitrary values. If we don't have such > permission we can change our uids only to the one of the current real, > effective or saved uid values, in the other case we will receive EPERM. > Actually, man setresuid states that :-[ > > setresuid() may return EINVAL, but I don't think we can raise it by > passing an unused uid value to setresuid() (if we have permission > setresuid() returns 0 if we don't - it returns EPERM). I was under an impression that there are some checks (possibly in libc) because the test comments talks about invalid uids and gids, but thinking about it this behavior makes more sense (and checking both kernel source and libc source indeed proves this). Sorry for the confusion. Note that the uid_valid() and gid_valid() checks are defined in include/linux/uidgid.h and can fail only if the value is ((kgid_t)-1) so as far as I can see the EINVAL cannot be triggered at all. And the same goes for uid. > So it's not bad that we are introducing tst_get_unused_uid, > tst_get_unused_gid functions and setting inval_user in a more formal way. > But It looks like passing inval_user value is the same as passing > any other uid != real|effective|saved uid of the current process.... > > Since we have in setresuid03 a full set for nobody uid: > setresuid(nobody, -1, -1) > setresuid(-1, -1, nobody) > setresuid(-1, nobody, -1) > what do you think about adding setresuid(inval_user, -1, -1) to the test > case? Just in case... > > Related to setregid02. > Even that test_data contains EINVAL for the last two members, setregid() > doesn't return EINVAL for them. It returns EPERM for all the test cases. > > You said that on SUSE setregid02 fails. I added a group with 65533 gid > and executed setregid02 (current git version) and it passed. Could you > check it again in your environment? I looked again at setregid02 it sets it's uid and gid (actually all of them because it uses setuid() and setgid() as a root) to nobody's uid and gid before the test starts and by a coincidence these are set to 65534 and 65533 on SUSE. Which explains the failure. The setresuid03 uses bin user instead. Now let's keep the code as it is for the release, it works good enough and fix these a better way after it. One of the options is to set all three uids/gids to known values and loop on all possible uids/gids and expect EPERM on everything but the ones we set previously. -- Cyril Hrubis ch...@su... |