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

[Andrey Ryabinin]

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@xxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/