linux-kernel - Re: [PATCH v1 1/2] PCI/sysfs: Change read permissions for VPD attributes

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241112063608.GF71181@unreal>
Date: Tue, 12 Nov 2024 08:36:08 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: Krzysztof Wilczyński <kw@...ux.com>,
	linux-pci@...r.kernel.org, Ariel Almog <ariela@...dia.com>,
	Aditya Prabhune <aprabhune@...dia.com>,
	Hannes Reinecke <hare@...e.de>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Arun Easi <aeasi@...vell.com>, Jonathan Chocron <jonnyc@...zon.com>,
	Bert Kenward <bkenward@...arflare.com>,
	Matt Carlson <mcarlson@...adcom.com>,
	Kai-Heng Feng <kai.heng.feng@...onical.com>,
	Jean Delvare <jdelvare@...e.de>,
	Alex Williamson <alex.williamson@...hat.com>,
	linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org Thomas Weißschuh <linux@...ssschuh.net>
Subject: Re: [PATCH v1 1/2] PCI/sysfs: Change read permissions for VPD
 attributes

On Mon, Nov 11, 2024 at 03:48:04PM -0600, Bjorn Helgaas wrote:
> [+cc Thomas]
> 
> On Mon, Nov 11, 2024 at 11:17:38PM +0200, Leon Romanovsky wrote:
> > On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote:
> > > On Thu, Nov 07, 2024 at 08:56:56PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro@...dia.com>
> > > > 
> > > > The Vital Product Data (VPD) attribute is not readable by regular
> > > > user without root permissions. Such restriction is not really needed
> > > > for many devices in the world, as data presented in that VPD is not
> > > > sensitive and access to the HW is safe and tested.
> > > > 
> > > > This change aligns the permissions of the VPD attribute to be accessible
> > > > for read by all users, while write being restricted to root only.
> > > > 
> > > > For the driver, there is a need to opt-in in order to allow this
> > > > functionality.
> > > 
> > > I don't think the use case is very strong (and not included at all
> > > here).
> > 
> > I will add the use case, which is running monitoring application without
> > need to be root. IMHO reducing number of applications that require
> > privileged access is a very strong case. I personally try to avoid
> > applications with root/setuid privileges.
> 
> Avoiding root/setuid is a very good thing.  I just don't think using
> VPD directly from userspace is a great idea because VPD is so slow and
> sometimes unreliable to read.

This is one time operation during application initialization, which is
fast in our devices. It is used by the NVML https://developer.nvidia.com/management-library-nvml.

> And apparently this is a pretty unusual situation since I haven't heard
> similar requests for other devices.

Maybe they didn't bother to ask.

> 
> Sort of ironic that some vendors, especially Intel and AMD, add new
> Device IDs for devices that work exactly the same as their
> predecessors, so we are continually adding to the pci_device_id
> tables, while here we apparently the same Device ID is used for
> devices that differ in ways we actually want to know about.

I'm not Intel or AMD employee and never worked there, but from what I
heard it is not technical decision but outcome of how their development
process is structured.

> 
> > > If we do need to do this, I think it's a property of the device, not
> > > the driver.
> > 
> > But how will device inform PCI core about safe VPD read?
> > Should I add new field to struct pci_device_id? Add a quirk?
> > Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true"
> > to mlx5 probe function and it won't change a lot from current
> > implementation.
> 
> To me it looks like a pci_dev bit, not a pci_driver bit.
> 
> I would set it .probe() so all the code is in one place.  And it's not
> related to a device defect, as most quirks are.

The advantage of quirks is that we will be able to set proper file
permissions from the beginning without need to load/bind driver.

As Thomas suggested, the vpd_attr_is_visible() will be something
like this, which is neat:

if (pdev->downgrade_vpd_read)
	return 644;
else
	return 600;

Thanks

> 
> Bjorn
>