| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a0944037-e90a-4884-b12f-284b373a0d63@linuxfoundation.org>
Date: Wed, 16 Oct 2024 17:10:20 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: zhouyuhang <zhouyuhang1010@....com>, brauner@...nel.org, shuah@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
zhouyuhang <zhouyuhang@...inos.cn>, Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH v2] selftests: clone3: Use the capget and capset syscall
directly
On 10/16/24 03:18, zhouyuhang wrote:
>
>
> 在 2024/10/15 23:31, Shuah Khan 写道:
>> On 10/15/24 04:59, zhouyuhang wrote:
>>> From: zhouyuhang <zhouyuhang@...inos.cn>
>>>
>>> The libcap commit aca076443591 ("Make cap_t operations thread safe.")
>>> added a __u8 mutex at the beginning of the struct _cap_struct, it changes
>>> the offset of the members in the structure that breaks the assumption
>>> made in the "struct libcap" definition in clone3_cap_checkpoint_restore.c.
>>> This will make the test fail. So use the capget and capset syscall
>>> directly and remove the libcap library dependency like the
>>> commit 663af70aabb7 ("bpf: selftests: Add helpers to directly use
>>> the capget and capset syscall") does.
>>>
>>> Signed-off-by: zhouyuhang <zhouyuhang@...inos.cn>
>>> ---
>>> tools/testing/selftests/clone3/Makefile | 1 -
>>> .../clone3/clone3_cap_checkpoint_restore.c | 53 ++++++++-----------
>>> .../selftests/clone3/clone3_cap_helpers.h | 23 ++++++++
>>> 3 files changed, 44 insertions(+), 33 deletions(-)
>>> create mode 100644 tools/testing/selftests/clone3/clone3_cap_helpers.h
>>>
>>> diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
>>> index 84832c369a2e..59d26e8da8d2 100644
>>> --- a/tools/testing/selftests/clone3/Makefile
>>> +++ b/tools/testing/selftests/clone3/Makefile
>>> @@ -1,6 +1,5 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES)
>>> -LDLIBS += -lcap
>>> TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
>>> clone3_cap_checkpoint_restore
>>> diff --git a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>> index 3c196fa86c99..242088eeec88 100644
>>> --- a/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>> +++ b/tools/testing/selftests/clone3/clone3_cap_checkpoint_restore.c
>>> @@ -15,7 +15,6 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <stdbool.h>
>>> -#include <sys/capability.h>
>>> #include <sys/prctl.h>
>>> #include <sys/syscall.h>
>>> #include <sys/types.h>
>>> @@ -26,6 +25,7 @@
>>> #include "../kselftest_harness.h"
>>> #include "clone3_selftests.h"
>>> +#include "clone3_cap_helpers.h"
>>> static void child_exit(int ret)
>>> {
>>> @@ -87,47 +87,36 @@ static int test_clone3_set_tid(struct __test_metadata *_metadata,
>>> return ret;
>>> }
>>> -struct libcap {
>>> - struct __user_cap_header_struct hdr;
>>> - struct __user_cap_data_struct data[2];
>>> -};
>>> -
>>> static int set_capability(void)
>>> {
>>> - cap_value_t cap_values[] = { CAP_SETUID, CAP_SETGID };
>>> - struct libcap *cap;
>>> - int ret = -1;
>>> - cap_t caps;
>>> -
>>> - caps = cap_get_proc();
>>> - if (!caps) {
>>> - perror("cap_get_proc");
>>> + struct __user_cap_data_struct data[2];
>>> + struct __user_cap_header_struct hdr = {
>>> + .version = _LINUX_CAPABILITY_VERSION_3,
>>> + };
>>> + __u32 cap0 = 1 << CAP_SETUID | 1 << CAP_SETGID;
>>> + __u32 cap1 = 1 << (CAP_CHECKPOINT_RESTORE - 32);
>>> + int ret;
>>> +
>>> + ret = capget(&hdr, data);
>>> + if (ret) {
>>> + perror("capget");
>>> return -1;
>>> }
>>> /* Drop all capabilities */
>>> - if (cap_clear(caps)) {
>>> - perror("cap_clear");
>>> - goto out;
>>> - }
>>> + memset(&data, 0, sizeof(data));
>>> - cap_set_flag(caps, CAP_EFFECTIVE, 2, cap_values, CAP_SET);
>>> - cap_set_flag(caps, CAP_PERMITTED, 2, cap_values, CAP_SET);
>>> + data[0].effective |= cap0;
>>> + data[0].permitted |= cap0;
>>> - cap = (struct libcap *) caps;
>>> + data[1].effective |= cap1;
>>> + data[1].permitted |= cap1;
>>> - /* 40 -> CAP_CHECKPOINT_RESTORE */
>>> - cap->data[1].effective |= 1 << (40 - 32);
>>> - cap->data[1].permitted |= 1 << (40 - 32);
>>> -
>>> - if (cap_set_proc(caps)) {
>>> - perror("cap_set_proc");
>>> - goto out;
>>> + ret = capset(&hdr, data);
>>> + if (ret) {
>>> + perror("capset");
>>> + return -1;
>>> }
>>> - ret = 0;
>>> -out:
>>> - if (cap_free(caps))
>>> - perror("cap_free");
>>> return ret;
>>> }
>>> diff --git a/tools/testing/selftests/clone3/clone3_cap_helpers.h b/tools/testing/selftests/clone3/clone3_cap_helpers.h
>>> new file mode 100644
>>> index 000000000000..3fa59ef68fb8
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/clone3/clone3_cap_helpers.h
>>> @@ -0,0 +1,23 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef __CLONE3_CAP_HELPERS_H
>>> +#define __CLONE3_CAP_HELPERS_H
>>> +
>>> +#include <linux/capability.h>
>>> +
>>> +/*
>>> + * Compatible with older version
>>> + * header file without defined
>>> + * CAP_CHECKPOINT_RESTORE.
>>> + */
>>> +#ifndef CAP_CHECKPOINT_RESTORE
>>> +#define CAP_CHECKPOINT_RESTORE 40
>>> +#endif
>>> +
>>> +/*
>>> + * Removed the libcap library dependency.
>>> + * So declare them here directly.
>>> + */
>>> +int capget(cap_user_header_t header, cap_user_data_t data);
>>> +int capset(cap_user_header_t header, const cap_user_data_t data);
>>
>> Sorry you haven't addressed my comments on your v1 yet.
>>
>> I repeat that this is not the right direction to define system
>> calls locally.
>>
>
> I got it. I am willing to modify the code so that syscalls are not defined in local files,
> but this would require including sys/capability.h which would not remove the
> dependency on the libcap library. So, should we directly use syscalls or use the
> libcap library function in the "set_capability" function, or do you have a better way.
> I'd like to refer to your advice.
>
>> Try this:
>>
>> Run make headers in the kernel repo.
>> Build without making any changes.
>> Then add you changes and add linux/capability.h to include files
>>
>> Tell me what happens.
>>
>> thanks,
>> -- Shuah
>
> I tried this, here are my steps.
>
> Firstly, I ran 'make headers' in the kernel repo and it was successful.
> Then I wasn't quite sure which path you were referring to as' build ',
Sorry if what I said wasn't clear:
- This test depends on libcap and yes you will have to install it.
- Run ake headers in the kernel repo.
- Build the test without your patch (changes)
- If you don't have libcap, the test build will fail
- Install libcap
- Build and run.
Looks like you have done the above. Now:
- Add your patch without the local capget() and capset()
and without removing
> so I compiled and installed libcap, and also compiled test, all of which were successful.
Why do you need to compile libcap? Is it because this latest
change isn't available to install from the distro you are using?
> Afterwards, I applied my patch and the test was successfully built and running.
> I guess what you're trying to express may be that these system calls have already
> been defined in sys/capability, and those defined in the local file are duplicated with it.
Correct. You don't need the local defines and in fact you should not
define them locally.
> So I included sys/capability.h and linux/capability.h and defined the system calls in the test,
> but there were no errors.
Please don't define system calls locally. What happens if you don't?
thanks,
-- Shuah
Powered by blists - more mailing lists