Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200911194549.12780-1-david.e.box@linux.intel.com>
 <20200911194549.12780-4-david.e.box@linux.intel.com> <0ec64bdc-66fd-4be1-03cf-561a7c42de68@linux.intel.com>
In-Reply-To: <0ec64bdc-66fd-4be1-03cf-561a7c42de68@linux.intel.com>
From:   Alexander Duyck <alexander.duyck@gmail.com>
Date:   Mon, 21 Sep 2020 06:36:57 -0700
Message-ID: <CAKgT0UdiXLhok1WOq9RoZKKi+f43xUoSBwX2LTYMpUUTU3HRwA@mail.gmail.com>
Subject: Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver
To:     Alexey Budankov <alexey.budankov@linux.intel.com>
Cc:     "David E. Box" <david.e.box@linux.intel.com>,
        Lee Jones <lee.jones@linaro.org>, dvhart@infradead.org,
        andy@infradead.org,
        Alexander Duyck <alexander.h.duyck@linux.intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        platform-driver-x86@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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