linux-kernel - Re: [PATCH v2] perf cap: Tidy up and improve capability testing

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAP-5=fUcuG7M6R80uwviwP2j62KDceDRRQM=Qf721bPBK1KYWg@mail.gmail.com>
Date: Tue, 30 Jul 2024 09:58:27 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	James Clark <james.clark@....com>, Oliver Upton <oliver.upton@...ux.dev>, 
	Leo Yan <leo.yan@...ux.dev>, Changbin Du <changbin.du@...wei.com>, 
	Athira Rajeev <atrajeev@...ux.vnet.ibm.com>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] perf cap: Tidy up and improve capability testing

On Tue, Jul 30, 2024 at 9:36 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Mon, Jul 29, 2024 at 11:19:31AM -0700, Ian Rogers wrote:
> > Remove dependence on libcap. libcap is only used to query whether a
> > capability is supported, which is just 1 capget system call.
> >
> > If the capget system call fails, fall back on root permission
> > checking. Previously if libcap fails then the permission is assumed
> > not present which may be pessimistic/wrong.
> >
> > Add a used_root out argument to perf_cap__capable to say whether the
> > fall back root check was used. This allows the correct error message,
> > "root" vs "users with the CAP_PERFMON or CAP_SYS_ADMIN capability", to
> > be selected.
> >
> > Tidy uses of perf_cap__capable so that tests aren't repeated if capget
> > isn't supported, to reduce calls or refactor similar to:
> > https://lore.kernel.org/lkml/20240729004127.238611-3-namhyung@kernel.org/
>
> I'm not familiar with the capability so it's hard to review the code but
> it'd be better to split the code for perf_cap__capable() and its usage.

There's an API change, passing the out arg if root checking was used,
so this would turn into a mess.

> > ---
> > v2: fix syscall number and '>' should have been '>='
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/Makefile.config  | 11 -------
> >  tools/perf/builtin-ftrace.c | 44 ++++++++++++--------------
> >  tools/perf/util/Build       |  2 +-
> >  tools/perf/util/cap.c       | 63 ++++++++++++++++++++++++++-----------
> >  tools/perf/util/cap.h       | 23 ++------------
> >  tools/perf/util/symbol.c    |  8 ++---
> >  tools/perf/util/util.c      | 12 +++++--
> >  7 files changed, 81 insertions(+), 82 deletions(-)
[snip]
> > --- a/tools/perf/util/cap.c
> > +++ b/tools/perf/util/cap.c
[snip]
> > +bool perf_cap__capable(int cap, bool *used_root)
> > +{
> > +     struct __user_cap_header_struct header = {
> > +             .version = _LINUX_CAPABILITY_VERSION_3,
> > +             .pid = getpid(),
> > +     };
> > +     struct __user_cap_data_struct data[MAX_LINUX_CAPABILITY_U32S];
> > +     __u32 cap_val;
> > +
> > +     *used_root = false;
> > +     while (syscall(SYS_capget, &header, &data[0]) == -1) {
> > +             /* Retry, first attempt has set the header.version correctly. */
> > +             if (errno == EINVAL && header.version != _LINUX_CAPABILITY_VERSION_3 &&
> > +                 header.version == _LINUX_CAPABILITY_VERSION_1)
>
> It seems you can just check it with _VERSION1.

Similarly not familiar. Basically there used to be a data struct of 3
u32s in v1, v2 shouldn't exist, v3 turned the data from an array of
structs of size [1] to [2]. v3 has been present for 16 years as shown
by its magic number 0x20080522 (v2.6.26 iirc - can't find the comment
again). It seems a pretty safe bet that v3 is the version being used.
In old kernels if the versions don't match they just return EINVAL and
you have to try again like this loop:
https://fossd.anu.edu.au/linux/v2.6.14/source/kernel/capability.c#L43
```
if (get_user(version, &header->version))
  return -EFAULT;

if (version != _LINUX_CAPABILITY_VERSION) {
  if (put_user(_LINUX_CAPABILITY_VERSION, &header->version))
    return -EFAULT;
   return -EINVAL;
}
```
So testing v3 is almost certainly what we want, what's going to work
and avoids a loop. In newer kernels the EINVAL isn't returned for a
mismatch and the code just does the right thing for whatever version
is requested.

> But I'm not sure about what this code does.  Who set the version
> correctly?  Is there any chance for an infinite loop?

The system call could return EINVAL with _LINUX_CAPABILITY_VERSION_1
and set the header.version to _LINUX_CAPABILITY_VERSION_1 again and
return EINVAL again, the kernel would be broken but it would lead to
an infinite loop.

Thanks,
Ian