| Message ID | 20210705212308.3050976-2-kw@linux.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [1/4] PCI/sysfs: Move to kstrtobool() to handle user input | expand |
On Mon, Jul 05, 2021 at 09:23:06PM +0000, Krzysztof Wilczyński wrote: > Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user > input as it makes more sense to first check whether the current user > actually has the right permissions before accepting any input from such > user. > > This will also make order in which enable_store() and msi_bus_store() > perform the "CAP_SYS_ADMIN" capability check consistent with other > PCI-related sysfs objects that first verify whether user has this > capability set. I like this one. Can you rebase it to skip patch 1/4 (unless you convince me that 1/4 is safe)? > Signed-off-by: Krzysztof Wilczyński <kw@linux.com> > --- > drivers/pci/pci-sysfs.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c > index 0f98c4843764..bc4c141e4c1c 100644 > --- a/drivers/pci/pci-sysfs.c > +++ b/drivers/pci/pci-sysfs.c > @@ -275,13 +275,13 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr, > struct pci_dev *pdev = to_pci_dev(dev); > ssize_t result = 0; > > - if (kstrtobool(buf, &enable) < 0) > - return -EINVAL; > - > /* this can crash the machine when done on the "wrong" device */ > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > + if (kstrtobool(buf, &enable) < 0) > + return -EINVAL; > + > device_lock(dev); > if (dev->driver) > result = -EBUSY; > @@ -377,12 +377,12 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr, > struct pci_dev *pdev = to_pci_dev(dev); > struct pci_bus *subordinate = pdev->subordinate; > > - if (kstrtobool(buf, &enable) < 0) > - return -EINVAL; > - > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > > + if (kstrtobool(buf, &enable) < 0) > + return -EINVAL; > + > /* > * "no_msi" and "bus_flags" only affect what happens when a driver > * requests MSI or MSI-X. They don't affect any drivers that have > -- > 2.32.0 >
Hi Bjorn, [...] > > Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user > > input as it makes more sense to first check whether the current user > > actually has the right permissions before accepting any input from such > > user. > > > > This will also make order in which enable_store() and msi_bus_store() > > perform the "CAP_SYS_ADMIN" capability check consistent with other > > PCI-related sysfs objects that first verify whether user has this > > capability set. > > I like this one. Can you rebase it to skip patch 1/4 (unless you > convince me that 1/4 is safe)? I will remove it, as per: https://lore.kernel.org/linux-pci/20210915011204.GA1444093@rocinante/T/#t Thank you! Krzysztof
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c index 0f98c4843764..bc4c141e4c1c 100644 --- a/drivers/pci/pci-sysfs.c +++ b/drivers/pci/pci-sysfs.c @@ -275,13 +275,13 @@ static ssize_t enable_store(struct device *dev, struct device_attribute *attr, struct pci_dev *pdev = to_pci_dev(dev); ssize_t result = 0; - if (kstrtobool(buf, &enable) < 0) - return -EINVAL; - /* this can crash the machine when done on the "wrong" device */ if (!capable(CAP_SYS_ADMIN)) return -EPERM; + if (kstrtobool(buf, &enable) < 0) + return -EINVAL; + device_lock(dev); if (dev->driver) result = -EBUSY; @@ -377,12 +377,12 @@ static ssize_t msi_bus_store(struct device *dev, struct device_attribute *attr, struct pci_dev *pdev = to_pci_dev(dev); struct pci_bus *subordinate = pdev->subordinate; - if (kstrtobool(buf, &enable) < 0) - return -EINVAL; - if (!capable(CAP_SYS_ADMIN)) return -EPERM; + if (kstrtobool(buf, &enable) < 0) + return -EINVAL; + /* * "no_msi" and "bus_flags" only affect what happens when a driver * requests MSI or MSI-X. They don't affect any drivers that have
Check if the "CAP_SYS_ADMIN" capability flag is set before parsing user input as it makes more sense to first check whether the current user actually has the right permissions before accepting any input from such user. This will also make order in which enable_store() and msi_bus_store() perform the "CAP_SYS_ADMIN" capability check consistent with other PCI-related sysfs objects that first verify whether user has this capability set. Signed-off-by: Krzysztof Wilczyński <kw@linux.com> --- drivers/pci/pci-sysfs.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)