On Mon, Dec 07, 2015 at 12:46:52PM +0100, Daniel Vetter wrote:
On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
On Mon, 07 Dec 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
On 12/06/2015 10:35 AM, Daniel Vetter wrote:
On 11/18/2015 06:58 PM, Nicolas Iooss wrote: > drm_dev_set_unique() formats its parameter using kvasprintf() but many > of its callers directly pass dev_name(dev) as printf format string, > without any format parameter. This can cause some issues when the > device name contains '%' characters. > > To avoid any potential issue, always use "%s" when using > drm_dev_set_unique() with dev_name().
Not sure this is worth it really, normally people don't place % characters into their device names, ever. And if they do it'll blow up. There's also no security issue here since userspace can't set this name.
If the maintainers of the affected drivers don't want this I won't merge this patch.
Actually I had the same opinion before I began to add __printf attributes and "%s" in several places in the kernel to make -Wformat-security useful. This led me to discover some funny issues like the one fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through user-controlled format string", https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3... ). The patch I sent is in fact a very small step towards making -Wformat-security useful again to detect "real" issues.
Of course, if you do not feel it is worth it and believe that dev_name is fully controlled by trusted sources which will never introduce any % character, I understand your will of not merging my patch.
Ah, that's the context I was missing, that really should be in the commit message. If this is part of an overall effort to enable something useful it makes more sense to get it in.
On the patch itself it feels rather funny to do a "%s", str); combo, maybe we should have a drm_dev_set_unique_static instead? Including kerneldoc explaining why there's too.
No caller of drm_dev_set_unique() actually uses the formatting for anything... so you'd end up with drm_dev_set_unique_static() and an orphaned drm_dev_set_unique()...
Ok, then I guess we can just ditch the printf stuff from set_unique. Nicolas, you're up for that?
Looking at all the callsites of drm_dev_set_unique() it seems like all of the drivers (with the exception of vgem) use dev_name() on the same device that's already passed into drm_dev_alloc(), so perhaps another alternative would be to have drm_dev_alloc() set the unique name by default and keep the function for cases where it needs to be set explicitly (like for vgem). vgem passes drm_dev_alloc() a NULL device, so that could serve as condition.
Thierry