On Sat, Sep 19, 2020 at 1:01 AM Alexey Budankov
<alexey.budankov@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> Thanks for the patches.
>
> On 11.09.2020 22:45, David E. Box wrote:
> > From: Alexander Duyck <alexander.h.duyck@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > ---
> > .../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
