linux-kernel - Re: [PATCH v2 2/3] fpga manager: framework core

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]

Date:	Sat, 6 Dec 2014 13:00:17 +0000
From:	Grant Likely <grant.likely@...aro.org>
To:	Pavel Machek <pavel@...x.de>
Cc:	atull@...nsource.altera.com,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	"H. Peter Anvin" <hpa@...or.com>, Michal Simek <monstr@...str.eu>,
	Michal Simek <michal.simek@...inx.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	Rob Herring <robh+dt@...nel.org>,
	Ira Snyder <iws@...o.caltech.edu>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Mark Brown <broonie@...nel.org>, philip@...ister.org,
	rubini <rubini@...dd.com>,
	Steffen Trumtrar <s.trumtrar@...gutronix.de>,
	Jason <jason@...edaemon.net>, kyle.teske@...com,
	Nicolas Pitre <nico@...aro.org>,
	"Balbi, Felipe" <balbi@...com>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	David Brown <davidb@...eaurora.org>,
	Rob Landley <rob@...dley.net>,
	David Miller <davem@...emloft.net>, cesarb@...arb.net,
	"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Alan Tull <delicious.quinoa@...il.com>,
	dinguyen@...nsource.altera.com,
	Yves Vandervennet <yvanderv@...nsource.altera.com>
Subject: Re: [PATCH v2 2/3] fpga manager: framework core

On Fri, Oct 24, 2014 at 11:52 AM, Pavel Machek <pavel@...x.de> wrote:
> Hi!
>
>> * /sys/class/fpga_manager/<fpga>/firmware
>>   Name of FPGA image file to load using firmware class.
>>   $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware
>
> I .. still don't think this is good idea. What about namespaces?
> The path corresponds to path in which namespace?

I don't understand your concern here. This allows userspace to name
the FPGA bitstream that the kernel will use during request_firmware(),
and it will show up as the $FIRMWARE value in the uevent file, but it
is still the responsibility of userspace to choose what to load, and
it can freely ignore the setting of $FIRMWARE if it needs to.

The process that actually loads the firmware into the kernel pretty
much has to run in the normal linux environment. How do namespaces
come into it? What exact problem do you see?

>
>> +int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
>> +{
>> +     int ret;
>> +
>> +     if (test_and_set_bit_lock(FPGA_MGR_BUSY, &mgr->flags))
>> +             return -EBUSY;
>> +
>> +     dev_info(mgr->dev, "writing buffer to %s\n", mgr->name);
>> +
>> +     ret = __fpga_mgr_write(mgr, buf, count);
>> +     clear_bit_unlock(FPGA_MGR_BUSY, &mgr->flags);
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_write);
>
> Is the EBUSY -- userspace please try again, but you don't know when to
> try again -- right interface? I mean, normally kernel would wait, so
> that userland does not have to poll?

Custom locking schemes are just wrong. A mutex is the right thing to
do here and then an -EBUSY isn't required.

>
>> +static ssize_t fpga_mgr_firmware_store(struct device *dev,
>> +                                    struct device_attribute *attr,
>> +                                    const char *buf, size_t count)
>> +{
>> +     struct fpga_manager *mgr = dev_get_drvdata(dev);
>> +     unsigned int len;
>> +     char image_name[NAME_MAX];

Hard coding a string length is a warning sign. That is the sort of
thing that can memdup() or strdup() can handle.

>> +     int ret;
>> +
>> +     /* lose terminating \n */
>> +     strcpy(image_name, buf);
>> +     len = strlen(image_name);
>> +     if (image_name[len - 1] == '\n')
>> +             image_name[len - 1] = 0;
>> +
>> +     ret = fpga_mgr_firmware_write(mgr, image_name);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return count;
>> +}
>
> This shows why the interface is not right... Valid filename may
> contain \n, right? It may even end with \n.

That's not an interface problem, it implementation problem. The
function absolutely should scrub and validate it's input. It should
also make sure that the string doesn't have any special characters so
that /bin/sh doesn't barf on it (because the string will appear in the
uevent file). I would check other users of request_firmware() to see
if any of them allow userspace to specify the filename.

That said, the other way to handle this is not to specify a valid
filename through this interface at all. Just use a dummy placeholder
name and expect userspace to load the correct file when the request is
posted.

g.
--
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/