linux-kernel - Re: [PATCH] lib: idr: fix out-of-bounds pointer dereference

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] [day] [month] [year] [list]

Date:	Tue, 24 Jun 2014 12:17:49 +0400
From:	Andrey Ryabinin <a.ryabinin@...sung.com>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Tejun Heo <tj@...nel.org>, linux-kernel@...r.kernel.org,
	tetra2005@...il.com, preobr@...gle.com, dvyukov@...gle.com,
	kcc@...gle.com, koct9i@...il.com
Subject: Re: [PATCH] lib: idr: fix out-of-bounds pointer dereference

On 06/24/14 11:48, Lai Jiangshan wrote:
> 
> 326cf0f0f308 ("idr: fix top layer handling") enlarged the pa array.
> But the additional "+1" space is only used in id-allocation, it is free
> in other usage, (paa may point to the additional "+1" space, but not dereference it).
> so you can reuse it.
> 
> In the 3 functions your patch touched:
> -	struct idr_layer ***paa = &pa[0];
> +	struct idr_layer ***paa = &pa[1];
> 

Yeah, I thought about such change, but decided this will look very confusing.
Though, this could be made less confusing with good comment why we are assigning
pointer to second element.
I'm think that we should also initialize pa[0] in such case, to avoid possible kmemchek's report
about uninitialized memory read.

> 
> I don't reject your patch, I had review it.
> 
> Reviewed-by: Lai Jiangshan <laijs@...fujitsu.com>
> 
> The reason why I'm still muttering here is that I wish a simple solution
> to fix the problem. And:
> 1) your patch also makes use of the additional "+1" @pa space: *++paa = p
> 2) your patch may slight enlarge the function body.
> 3) I think you patch reduces the readability a little although the idr code
>    itself is already shit.
> 

I have no strong opinion about what change is better. They both looks shitty to me.
The best solution here would be to rewrite this whole code from scratch.

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