From: Jean Delvare <khali@linux-fr.org>
To: Greg KH <greg@kroah.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>,
Milton Miller <miltonm@bga.com>,
Michael Ellerman <michael@ellerman.id.au>,
linux-kernel <linux-kernel@vger.kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
linux-pci@vger.kernel.org
Subject: Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful
Date: Sun, 17 Aug 2008 21:06:59 +0200 [thread overview]
Message-ID: <20080817210659.06601a3b@hyperion.delvare> (raw)
In-Reply-To: <20080816062259.GB20541@kroah.com>
Hi all,
On Fri, 15 Aug 2008 23:22:59 -0700, Greg KH wrote:
> On Fri, Aug 15, 2008 at 12:15:01PM -0700, Jesse Barnes wrote:
> > On Friday, August 15, 2008 11:55 am Jean Delvare wrote:
> > > In fact we can do even better than that. We could accept from
> > > user-space only driver_data values which at least one device ID entry in
> > > the driver already uses. That should be fairly easy to implement, and
> > > would offer a level of safety an order of magnitude above what we have
> > > at the moment... And it works both ways: if 0 is not a valid data for
> > > some driver, that would force the user to provide a non-zero (and
> > > valid) data value. And it guarantees that the user can't ask for
> > > something the driver doesn't expect, so drivers don't even need extra
> > > checks. And no need for a use_driver_data flag either.
> >
> > Meaning a driver audit of the usage? Yeah that would be great.
> >
> > > The only drawback is that it prevents the user from passing a "new"
> > > data value even if it would be valid. But honestly, I don't expect that
> > > case to happen frequently... if ever at all. So I'd say the benefits
> > > totally outweight the drawback.
> > >
> > > If the interested people agree with the idea, I'll look into
> > > implementing it.
> >
> > Well the audit would show if user supplied "new" values are needed; otherwise
> > the approach sounds good to me.
>
> That sounds reasonable, and should work properly.
>
> No objection from me.
Ok, here's what it could look like:
* * * * *
From: Jean Delvare <khali@linux-fr.org>
Subject: PCI: Check dynids driver_data value for validity
Only accept dynids those driver_data value matches one of the driver's
pci_driver_id entry. This prevents the user from accidentally passing
values the drivers do not expect.
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Milton Miller <miltonm@bga.com>
Cc: Greg KH <greg@kroah.com>
---
Documentation/PCI/pci.txt | 4 ++++
drivers/i2c/busses/i2c-amd756.c | 4 ----
drivers/i2c/busses/i2c-viapro.c | 4 ----
drivers/pci/pci-driver.c | 18 ++++++++++++++++--
4 files changed, 20 insertions(+), 10 deletions(-)
--- linux-2.6.27-rc3.orig/Documentation/PCI/pci.txt 2008-08-17 18:24:33.000000000 +0200
+++ linux-2.6.27-rc3/Documentation/PCI/pci.txt 2008-08-17 18:24:38.000000000 +0200
@@ -163,6 +163,10 @@ need pass only as many optional fields a
o class and classmask fields default to 0
o driver_data defaults to 0UL.
+Note that driver_data must match the value used by any of the pci_device_id
+entries defined in the driver. This makes the driver_data field mandatory
+if all the pci_device_id entries have a non-zero driver_data value.
+
Once added, the driver probe routine will be invoked for any unclaimed
PCI devices listed in its (newly updated) pci_ids list.
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-amd756.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-amd756.c 2008-08-17 19:42:14.000000000 +0200
@@ -332,10 +332,6 @@ static int __devinit amd756_probe(struct
int error;
u8 temp;
- /* driver_data might come from user-space, so check it */
- if (id->driver_data >= ARRAY_SIZE(chipname))
- return -EINVAL;
-
if (amd756_ioport) {
dev_err(&pdev->dev, "Only one device supported "
"(you have a strange motherboard, btw)\n");
--- linux-2.6.27-rc3.orig/drivers/i2c/busses/i2c-viapro.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/i2c/busses/i2c-viapro.c 2008-08-17 19:42:24.000000000 +0200
@@ -320,10 +320,6 @@ static int __devinit vt596_probe(struct
unsigned char temp;
int error = -ENODEV;
- /* driver_data might come from user-space, so check it */
- if (id->driver_data & 1 || id->driver_data > 0xff)
- return -EINVAL;
-
/* Determine the address of the SMBus areas */
if (force_addr) {
vt596_smba = force_addr & 0xfff0;
--- linux-2.6.27-rc3.orig/drivers/pci/pci-driver.c 2008-08-17 17:15:57.000000000 +0200
+++ linux-2.6.27-rc3/drivers/pci/pci-driver.c 2008-08-17 19:17:55.000000000 +0200
@@ -43,18 +43,32 @@ store_new_id(struct device_driver *drive
{
struct pci_dynid *dynid;
struct pci_driver *pdrv = to_pci_driver(driver);
+ const struct pci_device_id *ids = pdrv->id_table;
__u32 vendor, device, subvendor=PCI_ANY_ID,
subdevice=PCI_ANY_ID, class=0, class_mask=0;
unsigned long driver_data=0;
int fields=0;
- int retval = 0;
+ int retval;
- fields = sscanf(buf, "%x %x %x %x %x %x %lux",
+ fields = sscanf(buf, "%x %x %x %x %x %x %lx",
&vendor, &device, &subvendor, &subdevice,
&class, &class_mask, &driver_data);
if (fields < 2)
return -EINVAL;
+ /* Only accept driver_data values that match an existing id_table
+ entry */
+ retval = -EINVAL;
+ while (ids->vendor || ids->subvendor || ids->class_mask) {
+ if (driver_data == ids->driver_data) {
+ retval = 0;
+ break;
+ }
+ ids++;
+ }
+ if (retval) /* No match */
+ return retval;
+
dynid = kzalloc(sizeof(*dynid), GFP_KERNEL);
if (!dynid)
return -ENOMEM;
* * * * *
The patch above applies on top of Milton's patch removing
dynids.use_driver_data.
Note that I fixed a bug in the code: the driver_data value was scanned
with format "%lux" which isn't valid. It's either "%lu" or "%lx". It
went unnoticed so far because it's the last field. I've made it "%lx"
because that's what our documentation says it should be. The old code
was behaving like "%lu" instead, so that's an interface change. But I
doubt it matters much, given that only 3 drivers were using this field
so far.
As mentioned before, this safety check might be too tight in some
cases (for example if driver_data is a bit field and the new device
needs a flag combination that no supported device uses.) It wouldn't be
difficult to let drivers set a flag saying they will care about the
safety check themselves. I can even implement it now if anybody thinks
that my code is too restrictive.
Thanks,
--
Jean Delvare
next prev parent reply other threads:[~2008-08-17 19:07 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-07-10 21:12 [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region Milton Miller
2008-07-10 21:14 ` mtd: remove __PPC__ hardcoded address from nand/diskonchip and devices/docprobe Milton Miller
2008-07-10 21:33 ` [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region Hans de Goede
2008-07-10 21:51 ` Milton Miller
2008-07-11 6:52 ` Jean Delvare
2008-07-11 7:27 ` Hans de Goede
2008-07-11 7:36 ` Jean Delvare
2008-07-13 6:31 ` Hans de Goede
2008-07-13 21:11 ` [lm-sensors] " David Hubbard
2008-07-13 21:22 ` Hans de Goede
2008-07-13 21:26 ` David Hubbard
2008-07-14 7:59 ` Jean Delvare
2008-07-14 17:09 ` Milton Miller
2008-07-14 17:30 ` [lm-sensors] " Hans de Goede
2008-07-14 17:55 ` David Hubbard
2008-07-15 8:36 ` Jean Delvare
2008-07-15 15:31 ` David Hubbard
2008-07-16 7:46 ` Jean Delvare
2008-07-16 8:09 ` Rene Herman
2008-07-15 8:28 ` Jean Delvare
[not found] ` <for-27-patch9@bga.com>
2008-07-12 20:02 ` [PATCH/RESEND] pci: dynids.use_driver_data considered harmful Milton Miller
2008-07-12 20:17 ` Greg KH
2008-07-12 20:58 ` Jean Delvare
2008-07-12 21:17 ` Milton Miller
2008-07-12 21:29 ` Milton Miller
[not found] ` <20080712041137.GA5933@kroah.com>
2008-07-12 21:08 ` [PATCH/RFC] " Milton Miller
2008-07-12 22:48 ` Milton Miller
2008-07-16 10:18 ` Milton Miller
2008-07-17 7:07 ` Greg KH
2008-07-17 14:36 ` Milton Miller
2008-08-06 7:31 ` Jean Delvare
2008-08-14 22:12 ` Greg KH
2008-08-15 14:50 ` Milton Miller
2008-08-15 15:50 ` Jean Delvare
2008-08-15 17:46 ` Jesse Barnes
2008-08-15 18:55 ` Jean Delvare
2008-08-15 19:15 ` Jesse Barnes
2008-08-16 6:22 ` Greg KH
2008-08-17 19:06 ` Jean Delvare [this message]
2008-08-18 3:50 ` Greg KH
2008-08-18 17:13 ` Jesse Barnes
2008-08-18 20:41 ` Jesse Barnes
2008-08-19 18:01 ` Milton Miller
2008-08-06 7:22 ` Jean Delvare
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080817210659.06601a3b@hyperion.delvare \
--to=khali@linux-fr.org \
--cc=akpm@linux-foundation.org \
--cc=greg@kroah.com \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=michael@ellerman.id.au \
--cc=miltonm@bga.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).