[BUGFIX PATCH v2 0/3] selftests: safesetid: Fix some bugs in safesetid test

[BUGFIX PATCH v2 0/3] selftests: safesetid: Fix some bugs in safesetid test

[Masami Hiramatsu]

Hi,

Here is the v2 series to fix build warnings and erorrs on
kselftest safesetid.
This version includes a fix for a runtime error.

Thank you,

---

Masami Hiramatsu (3):
      selftests: safesetid: Move link library to LDLIBS
      selftests: safesetid: Check the return value of setuid/setgid
      selftests: safesetid: Fix Makefile to set correct test program


 tools/testing/selftests/safesetid/Makefile         |    5 +++--
 tools/testing/selftests/safesetid/safesetid-test.c |   15 ++++++++++-----
 2 files changed, 13 insertions(+), 7 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[BUGFIX PATCH v2 1/3] selftests: safesetid: Move link library to LDLIBS

[Masami Hiramatsu]

Move -lcap to LDLIBS from CFLAGS because it is a library
to be linked.

Without this, safesetid failed to build with link error
as below.

----
/usr/bin/ld: /tmp/ccL8rZHT.o: in function `drop_caps':
safesetid-test.c:(.text+0xe7): undefined reference to `cap_get_proc'
/usr/bin/ld: safesetid-test.c:(.text+0x107): undefined reference to `cap_set_flag'
/usr/bin/ld: safesetid-test.c:(.text+0x10f): undefined reference to `cap_set_proc'
/usr/bin/ld: safesetid-test.c:(.text+0x117): undefined reference to `cap_free'
/usr/bin/ld: safesetid-test.c:(.text+0x136): undefined reference to `cap_clear'
collect2: error: ld returned 1 exit status
----

Fixes: c67e8ec03f3f ("LSM: SafeSetID: add selftest")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/testing/selftests/safesetid/Makefile |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/safesetid/Makefile b/tools/testing/selftests/safesetid/Makefile
index 98da7a504737..cac42cd36a1b 100644
--- a/tools/testing/selftests/safesetid/Makefile
+++ b/tools/testing/selftests/safesetid/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for mount selftests.
-CFLAGS = -Wall -lcap -O2
+CFLAGS = -Wall -O2
+LDLIBS = -lcap
 
 TEST_PROGS := run_tests.sh
 TEST_GEN_FILES := safesetid-test

[BUGFIX PATCH v2 2/3] selftests: safesetid: Check the return value of setuid/setgid

[Masami Hiramatsu]

Check the return value of setuid() and setgid().
This fixes the following warnings and improves test result.

safesetid-test.c: In function ‘main’:
safesetid-test.c:294:2: warning: ignoring return value of ‘setuid’, declared with attribute warn_unused_result [-Wunused-result]
  setuid(NO_POLICY_USER);
  ^~~~~~~~~~~~~~~~~~~~~~
safesetid-test.c:295:2: warning: ignoring return value of ‘setgid’, declared with attribute warn_unused_result [-Wunused-result]
  setgid(NO_POLICY_USER);
  ^~~~~~~~~~~~~~~~~~~~~~
safesetid-test.c:309:2: warning: ignoring return value of ‘setuid’, declared with attribute warn_unused_result [-Wunused-result]
  setuid(RESTRICTED_PARENT);
  ^~~~~~~~~~~~~~~~~~~~~~~~~
safesetid-test.c:310:2: warning: ignoring return value of ‘setgid’, declared with attribute warn_unused_result [-Wunused-result]
  setgid(RESTRICTED_PARENT);
  ^~~~~~~~~~~~~~~~~~~~~~~~~
safesetid-test.c: In function ‘test_setuid’:
safesetid-test.c:216:3: warning: ignoring return value of ‘setuid’, declared with attribute warn_unused_result [-Wunused-result]
   setuid(child_uid);
   ^~~~~~~~~~~~~~~~~

Fixes: c67e8ec03f3f ("LSM: SafeSetID: add selftest")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/testing/selftests/safesetid/safesetid-test.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
index 8f40c6ecdad1..0c4d50644c13 100644
--- a/tools/testing/selftests/safesetid/safesetid-test.c
+++ b/tools/testing/selftests/safesetid/safesetid-test.c
@@ -213,7 +213,8 @@ static void test_setuid(uid_t child_uid, bool expect_success)
 	}
 
 	if (cpid == 0) {	    /* Code executed by child */
-		setuid(child_uid);
+		if (setuid(child_uid) < 0)
+			exit(EXIT_FAILURE);
 		if (getuid() == child_uid)
 			exit(EXIT_SUCCESS);
 		else
@@ -291,8 +292,10 @@ int main(int argc, char **argv)
 
 	// First test to make sure we can write userns mappings from a user
 	// that doesn't have any restrictions (as long as it has CAP_SETUID);
-	setuid(NO_POLICY_USER);
-	setgid(NO_POLICY_USER);
+	if (setuid(NO_POLICY_USER) < 0)
+		die("Error with set uid(%d)\n", NO_POLICY_USER);
+	if (setgid(NO_POLICY_USER) < 0)
+		die("Error with set gid(%d)\n", NO_POLICY_USER);
 
 	// Take away all but setid caps
 	drop_caps(true);
@@ -306,8 +309,10 @@ int main(int argc, char **argv)
 		die("test_userns failed when it should work\n");
 	}
 
-	setuid(RESTRICTED_PARENT);
-	setgid(RESTRICTED_PARENT);
+	if (setuid(RESTRICTED_PARENT) < 0)
+		die("Error with set uid(%d)\n", RESTRICTED_PARENT);
+	if (setgid(RESTRICTED_PARENT) < 0)
+		die("Error with set gid(%d)\n", RESTRICTED_PARENT);
 
 	test_setuid(ROOT_USER, false);
 	test_setuid(ALLOWED_CHILD1, true);

[BUGFIX PATCH v2 3/3] selftests: safesetid: Fix Makefile to set correct test program

[Masami Hiramatsu]

Fix Makefile to set safesetid-test.sh to TEST_PROGS instead
of non existing run_tests.sh.

Without this fix, I got following error.
  ----
  TAP version 13
  1..1
  # selftests: safesetid: run_tests.sh
  # Warning: file run_tests.sh is missing!
  not ok 1 selftests: safesetid: run_tests.sh
  ----

Fixes: c67e8ec03f3f ("LSM: SafeSetID: add selftest")
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/testing/selftests/safesetid/Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/safesetid/Makefile b/tools/testing/selftests/safesetid/Makefile
index cac42cd36a1b..fa02c4d5ec13 100644
--- a/tools/testing/selftests/safesetid/Makefile
+++ b/tools/testing/selftests/safesetid/Makefile
@@ -3,7 +3,7 @@
 CFLAGS = -Wall -O2
 LDLIBS = -lcap
 
-TEST_PROGS := run_tests.sh
+TEST_PROGS := safesetid-test.sh
 TEST_GEN_FILES := safesetid-test
 
 include ../lib.mk

Re: [BUGFIX PATCH v2 0/3] selftests: safesetid: Fix some bugs in safesetid test

[Micah Morton]

On Thu, Dec 5, 2019 at 4:20 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi,
>
> Here is the v2 series to fix build warnings and erorrs on
> kselftest safesetid.
> This version includes a fix for a runtime error.
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (3):
>       selftests: safesetid: Move link library to LDLIBS
>       selftests: safesetid: Check the return value of setuid/setgid
>       selftests: safesetid: Fix Makefile to set correct test program

These 3 fixes look good, thanks. Were you thinking they would go
through my SafeSetID tree or is there a dedicated one for selftests? I
guess if you're not sure someone else on here can chime in, or I can
just take them through my tree if I don't hear anything.

>
>
>  tools/testing/selftests/safesetid/Makefile         |    5 +++--
>  tools/testing/selftests/safesetid/safesetid-test.c |   15 ++++++++++-----
>  2 files changed, 13 insertions(+), 7 deletions(-)
>
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

Re: [BUGFIX PATCH v2 0/3] selftests: safesetid: Fix some bugs in safesetid test

[shuah]

On 12/5/19 9:40 AM, Micah Morton wrote:
> On Thu, Dec 5, 2019 at 4:20 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>
>> Hi,
>>
>> Here is the v2 series to fix build warnings and erorrs on
>> kselftest safesetid.
>> This version includes a fix for a runtime error.
>>
>> Thank you,
>>
>> ---
>>
>> Masami Hiramatsu (3):
>>        selftests: safesetid: Move link library to LDLIBS
>>        selftests: safesetid: Check the return value of setuid/setgid
>>        selftests: safesetid: Fix Makefile to set correct test program
> 
> These 3 fixes look good, thanks. Were you thinking they would go
> through my SafeSetID tree or is there a dedicated one for selftests? I
> guess if you're not sure someone else on here can chime in, or I can
> just take them through my tree if I don't hear anything.
> 

Yes. There is a linux-kselftest tree dedicated to selftests.
I can take them.


>>
>>
>>   tools/testing/selftests/safesetid/Makefile         |    5 +++--
>>   tools/testing/selftests/safesetid/safesetid-test.c |   15 ++++++++++-----
>>   2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> --

thanks,
-- Shuah

Re: [BUGFIX PATCH v2 0/3] selftests: safesetid: Fix some bugs in safesetid test

[Masami Hiramatsu]

On Thu, 5 Dec 2019 09:44:52 -0700
shuah <shuah@kernel.org> wrote:

> On 12/5/19 9:40 AM, Micah Morton wrote:
> > On Thu, Dec 5, 2019 at 4:20 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >>
> >> Hi,
> >>
> >> Here is the v2 series to fix build warnings and erorrs on
> >> kselftest safesetid.
> >> This version includes a fix for a runtime error.
> >>
> >> Thank you,
> >>
> >> ---
> >>
> >> Masami Hiramatsu (3):
> >>        selftests: safesetid: Move link library to LDLIBS
> >>        selftests: safesetid: Check the return value of setuid/setgid
> >>        selftests: safesetid: Fix Makefile to set correct test program
> > 
> > These 3 fixes look good, thanks. Were you thinking they would go
> > through my SafeSetID tree or is there a dedicated one for selftests? I
> > guess if you're not sure someone else on here can chime in, or I can
> > just take them through my tree if I don't hear anything.
> > 
> 
> Yes. There is a linux-kselftest tree dedicated to selftests.
> I can take them.

Thanks Micah and Shuah!

> 
> 
> >>
> >>
> >>   tools/testing/selftests/safesetid/Makefile         |    5 +++--
> >>   tools/testing/selftests/safesetid/safesetid-test.c |   15 ++++++++++-----
> >>   2 files changed, 13 insertions(+), 7 deletions(-)
> >>
> >> --
> 
> thanks,
> -- Shuah
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>