linux-kernel - Re: [PATCH] posix-timer: don't call idr

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:	Wed, 20 Feb 2013 22:38:36 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Tejun Heo <tj@...nel.org>
cc:	akpm@...ux-foundation.org, Sasha Levin <sasha.levin@...cle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] posix-timer: don't call idr_find() w/ negative ID

On Wed, 20 Feb 2013, Tejun Heo wrote:

> Recent idr updates make idr_find() trigger WARN_ON_ONCE() before
> returning NULL when a negative ID is specified.  Apparently,
> posix-timer::__lock_timer() was depending on idr_find() returning NULL
> on negative ID, thus triggering the new WARN_ON_ONCE().  Make
> __lock_timer() first check whether @timer_id is negative and return
> NULL without invoking idr_find() if so.

I can grumpily accept the patch below as a quick hack fix, which can
go to stable as well, but not with such a patently misleading
changelog.

The changelog wants to document, that this is not a proper fix at all
and just a quick hack which can be nonintrusively applied to stable.

> Note that the previous code was theoretically broken.  idr_find()
> masked off the sign bit before performing lookup and if the matching
> IDs were in use, it would have returned pointer for the incorrect
> entry.

Brilliant code that. What's the purpose of having the idr id as an
"int" and then masking off the sign bit instead of simply refusing
negative id values in the idr code itself or simply making the id
"unsigned int" ?

Just look at the various f*ckedup users of MAX_IDR_MASK. Example:

drivers/pps/pps.c:

        err = idr_get_new(&pps_idr, pps, &pps->id);
        mutex_unlock(&pps_idr_lock);

        if (err < 0)
                return err;

        pps->id &= MAX_IDR_MASK;

Why the heck is that necessary? Either we get an error code and we
don't care about pps->id or idr_get_new() sets pps->id to a valid idr
id. If the idr code really returns a _valid_ id with the sign bit set,
then the idr code is broken beyond repair and needs to be fixed
instead of propagating that insanity all over the users.

Thanks,

	tglx

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