linux-kernel - Re: [PATCH] drm: make variable named "refcount" atomic, like most refcounts in the kernel.

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:	Sun, 27 Apr 2014 00:00:56 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Lionel Debroux <lionel_debroux@...oo.fr>
Cc:	Mateusz Guzik <mguzik@...hat.com>, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] drm: make variable named "refcount" atomic, like most
 refcounts in the kernel.

On Sat, Apr 26, 2014 at 10:09:49PM +0200, Lionel Debroux wrote:

> > I believe this change is in grsecurity so that overflow detector can
> > be used,
> That's my understanding as well.
> > there is clearly no reason to use mere atomic ops.
> Yeah, sorry. At least, you're stating it in a nice way.

Which clearly does not work either.

> > It may be that kernel devs would accept a patch implementing generic
> > refcount manipulation primitives without atomicity guarantees, which
> > could be used in cases like this.
> > 
> > Then atomic and non-atomic versions could be used to detect
> > overflows and overputs at least in debug kernels.
> That's a more constructive suggestion indeed, on a useful feature :)

	So more detailed explanation is in order.  Very well.

	You really need to find out what grsec changes in atomic_inc()
semantics.  Because semantics is changed, and simple "grsec folks say
it's an improvement" does *NOT* translate into "let's do that change
everywhere".  Non-atomic variant would be
	if (++*p < 0) {
		--*p;
		whine
		send SIGKILL to ourselves
	}
which is nowhere near a sane mitigation in this case.  Much saner one would
be (*IF* the overflow is, indeed, possible and if simple s/int/long/ wouldn't
be a better fix) something along the lines of
	mutex_lock(&item->mutex);
	if (unlikely(item->refcount == INT_MAX)) {
		ret = -EINVAL;
		<maybe whine and/or raise SIGKILL>
		goto out_err;
	}
	<what we do there right now>

Mind you, I would be quite surprised if it turned out to be even
theoretically possible to get that overflow here (judging by the look
of callers, you'll run out of device numbers long before), but that's
a separate story.

The point is, your "useful feature" is anything but, when applied without
careful analysis of the situation; it's *not* a universal improvement.
And I stand by the cargo cult comment; you've posted a patch that, aside of
being completely pointless from the atomicity point of view, had done
nothing whatsoever on the mainline kernel.  Moreover, even if we had that
kind of atomic_inc semantics, your change would at the very least require
some analysis of the surrounding code.  Which you have replaced with
"grsec folks had done it that way".  Great, and maybe they even had done
that review, but then you should've asked them to give it to you - or do it
yourself if you don't want to bother them.
--
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/