linux-kernel - Re: [PATCH v2 3/5] firmware: check for possible file truncation early

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:	Tue, 12 May 2015 23:06:21 +0200
From:	"Luis R. Rodriguez" <mcgrof@...e.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Luis R. Rodriguez" <mcgrof@...not-panic.com>,
	Ming Lei <ming.lei@...onical.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	David Howells <dhowells@...hat.com>,
	Seth Forshee <seth.forshee@...onical.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Paul Bolle <pebolle@...cali.nl>,
	Linux Wireless List <linux-wireless@...r.kernel.org>,
	Kyle McMartin <kyle@...nel.org>
Subject: Re: [PATCH v2 3/5] firmware: check for possible file truncation early

On Tue, May 12, 2015 at 01:35:59PM -0700, Linus Torvalds wrote:
> On Tue, May 12, 2015 at 11:30 AM, Luis R. Rodriguez
> <mcgrof@...not-panic.com> wrote:
> >
> > Instead of waiting until the last second to fail
> > on a request_firmware*() calls due to filename
> > truncation we can do an early check upon boot
> > and immediatley avoid any possible issues upfront.
> 
> Why? This looks stupid. Why add this special case, when normal path
> lookup results in the proper errors

It seemed silly to proceed late if we can catch the possible name errors
early. It does indeed have the cost of all that early cruft code.

> And if invalid pathnames are so frequent that this special case is
> somehow worth it, we should fix whoever generates that crap, instead
> of adding this insane special case.

OK, I'm all for ignoring non-upstream drivers.

> And if we don't handle the errors from normal path lookup properly,
> then we should fix *that*.

That was done on patch 2, originally I was going for a simple early
check which can be put on all API calls prior to doing anything too
intrusive:

+static int sysdata_validate_filename(const char *name)
+{
+       if (!name)
+               return -EINVAL;
+       /* POSIX.1 2.4: an empty pathname is invalid, match other checks */
+       if (name[0] == '\0')
+               return -ENOENT;
+}

Since the truncation was possible too though it seemed worthy to add given
that quite a few callers can end up re-using the same code.

> In other words, I see absolutely no reason for this patch. Regardless
> of the reason for it, it seems entirely broken.

OK I'll get rid of all these early checks.

  Luis
--
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/