linux-kernel - Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 21 Sep 2020 06:36:57 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Alexey Budankov <alexey.budankov@...ux.intel.com>
Cc:     "David E. Box" <david.e.box@...ux.intel.com>,
        Lee Jones <lee.jones@...aro.org>, dvhart@...radead.org,
        andy@...radead.org,
        Alexander Duyck <alexander.h.duyck@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver

On Sat, Sep 19, 2020 at 1:01 AM Alexey Budankov
<alexey.budankov@...ux.intel.com> wrote:
>
> Hi,
>
> Thanks for the patches.
>
> On 11.09.2020 22:45, David E. Box wrote:
> > From: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
> >
> > Add support for the Intel Platform Monitoring Technology crashlog
> > interface.  This interface provides a few sysfs values to allow for
> > controlling the crashlog telemetry interface as well as a character driver
> > to allow for mapping the crashlog memory region so that it can be accessed
> > after a crashlog has been recorded.
> >
> > This driver is meant to only support the server version of the crashlog
> > which is identified as crash_type 1 with a version of zero. Currently no
> > other types are supported.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
> > Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> > ---
> >  .../ABI/testing/sysfs-class-pmt_crashlog      |  66 ++
> >  drivers/platform/x86/Kconfig                  |  10 +
> >  drivers/platform/x86/Makefile                 |   1 +
> >  drivers/platform/x86/intel_pmt_crashlog.c     | 588 ++++++++++++++++++
> >  4 files changed, 665 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-pmt_crashlog
> >  create mode 100644 drivers/platform/x86/intel_pmt_crashlog.c
>
> <SNIP>
>
> > +
> > +/*
> > + * devfs
> > + */
> > +static int pmt_crashlog_open(struct inode *inode, struct file *filp)
> > +{
> > +     struct crashlog_entry *entry;
> > +     struct pci_driver *pci_drv;
> > +     struct pmt_crashlog_priv *priv;
> > +
> > +     if (!capable(CAP_SYS_ADMIN))
> > +             return -EPERM;
>
> Will not this above still block access to /dev/crashlogX for admin_group users
> in case root configured access e.g. similar to this:
>
> ls -alh /dev/
> crw-rw----.  1 root admin_group      1,   9 Sep 15 18:28 crashlogX
>
> If yes then that capable() check is probably superfluous and
> should be avoided in order not to block access to PMT data.
>
> Could you please clarify or comment?
>
> Thanks,
> Alexei

Actually this should probably be updated to "if (!perfmon_capable())"
instead. The telemetry driver code originally had the CAP_SYS_ADMIN
check and it probably makes more sense to limit this user-wise to the
same users who have access to performon.

Thanks.

- Alex