linux-kernel - Re: [PATCH] firmware: speed up request

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: <49D9C0B0.1060905@ladisch.de>
Date:	Mon, 06 Apr 2009 10:43:28 +0200
From:	Clemens Ladisch <clemens@...isch.de>
To:	Andrey Borzenkov <arvidjaar@...l.ru>
CC:	Ira Snyder <iws@...o.caltech.edu>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: speed up request_firmware()

Andrey Borzenkov wrote:
> On Friday 03 of April 2009 10:46:11 Clemens Ladisch wrote:
> > Well, userspace does know the actual size of the image, so I see no
> > reason why it shouldn't be able to tell the kernel about it
> > beforehand.
> 
> Right; but it will need some time for user space to catch up.

It's only an optimization.

> > @@ -46,6 +46,7 @@ struct firmware_priv {
> >  	struct firmware *fw;
> >  	unsigned long status;
> >  	int alloc_size;
> > +	int size_hint;
> 
> Unsigned?

It is as signed as alloc_size.  ;-)

Yes, all these variables probably should be size_t.  

> > +static ssize_t firmware_data_size_show(struct device *dev,
> > +				       struct device_attribute *attr, char *buf)
> > +{
> > +	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", fw_priv->size_hint);
> > +}
> 
> Why would you need it?

Good question.  Apparently, for the same reason why we'd need
firmware_data_read ...

> > +static ssize_t firmware_data_size_store(struct device *dev,
> > +					struct device_attribute *attr,
> > +					const char *buf, size_t count)
> > +{
> > +	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
> > +	long value;
> > +	int err;
> > +
> > +	err = strict_strtol(buf, 10, &value);
> > +	if (err)
> > +		return err;
> > +	fw_priv->size_hint = value;
> 
> Should not there be some protection against using silly large values? 

It is already possible to do "cat /dev/zero > .../data".  What we'd need
is a limit not only on this variable but on the size of the firmware
image itself.


Okay, I'll do a bunch of patches to fix these warts in the firmware
loader.


Best regards,
Clemens
--
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/