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

From: Alexey Budankov <alexey.budankov@linux.intel.com>
To: "David E. Box" <david.e.box@linux.intel.com>,
	lee.jones@linaro.org, dvhart@infradead.org, andy@infradead.org,
	alexander.h.duyck@linux.intel.com
Cc: linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org,
	Alexey Budankov <alexey.budankov@linux.intel.com>
Subject: Re: [PATCH 3/3] platform/x86: Intel PMT Crashlog capability driver
Date: Sat, 19 Sep 2020 10:58:38 +0300	[thread overview]
Message-ID: <0ec64bdc-66fd-4be1-03cf-561a7c42de68@linux.intel.com> (raw)
In-Reply-To: <20200911194549.12780-4-david.e.box@linux.intel.com>

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  