linux-kernel - Re: [PATCH/RFC] pci: dynids.use_driver

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: <200808151046.59590.jbarnes@virtuousgeek.org>
Date:	Fri, 15 Aug 2008 10:46:59 -0700
From:	Jesse Barnes <jbarnes@...tuousgeek.org>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	Greg KH <greg@...ah.com>, Milton Miller <miltonm@....com>,
	Michael Ellerman <michael@...erman.id.au>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH/RFC] pci: dynids.use_driver_data considered harmful

> > It's pretty "trivial" to look to see if the field is set in the pci_id
> > structure, that should be all that is needed, right?
>
> Well... If you consider that all drivers using the driver_data field
> should have the dynids.use_driver_data flag set unconditionally and
> without looking at the code, then in fact you agree with Milton and
> myself that this flag should be dropped. The whole point of the flag,
> if my guess is correct, was to hint the developer that he/she should
> double check his/her code before setting the flag. If you set it for
> all these drivers, then all it does is preventing users from passing a
> driver_data value to drivers which do _not_ use the driver_data... but
> doing that has zero effect at the moment, so that wouldn't actually be
> any different from dropping dynids.use_driver_data altogether.
>
> So I have to reiterate my support to Milton's idea of dropping
> the dynids.use_driver_data flag. It does more harm than good.

Yeah it doesn't seem to be very heavily used at this point. :)

I see around 400 uses of id->driver_data right now, and only 2 of 
use_driver_data.  If we remove the use_driver_data field, drivers (except for 
the 2 using the field) will start to see whatever values userspace passes to 
them rather than 0, and at least in some of the few cases I looked at that 
could cause breakage due to out of bounds references.

So I think your point about dynamic IDs in general is a good one; this flag 
really does look like an "audit was done" bit, but doesn't go as far is it 
should.

The patch you posted to forbid dynamic binding unless use_driver_data is iset 
is probably the safest thing to do, given that drivers that *don't* set 
use_driver_data look like they might misbehave even with a driver_data value 
of 0...

What do you think, Greg?  Convinced yet? :)

Thanks,
Jesse
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/