Re: [patch] validate binary before use

Jan Kratochvil
2013-02-01T03:06:28Z

On Thu, 31 Jan 2013 15:23:07 +0100, Aleksandar Ristovski wrote:
> On 13-01-30 02:16 PM, Jan Kratochvil wrote:
> >On Tue, 29 Jan 2013 17:15:13 +0100, Aleksandar Ristovski wrote:
> >>+ /* Section vma is unrelocated. If SO_BASE_ADDR is zero, then
> >>+ use ASECT->VMA as-is. If not, then use offset + base addr. */
> >>+ res = target_verify_memory (data, (so_base_addr > 0)?
> >
> >I do not see why to use target_verify_memory in this case.
>
> While your comment below is correct, I find, since we introduced
> target_verify_memory already, this to be "more correct". Well, it is
> equivalent to what you are suggesting and I was considering doing
> simply read_memory/memcmp here, but then figured,
> target_verify_memory is more semantically correct.
>
> >
> >target_verify_memory is there for large sections to compare only their 32-bit
> >checksum. But build-id is already only 20 bytes long, with the protocol
> >overhead the 4 vs. 20 bytes do not make a difference. And it needlessly
> >weakens the check, it also does some patching of target_verify_memory.
> >Just use target_read_memory and memcmp.
>
> This is all true; however my opinion is that fallback in
> target_verify_memory is correct implementation as it allows using
> target_verify_memory where semantically suitable (like this place
> IMO) regardless of whether actual target implements it or not (e.g.
> core doesn't need to implement it; if it did, the implementation
> would probably be exactly the same as the fallback).

This is a bit nitpicking, primarily I do not see there much difference and we
avoid dealing with target_verify_memory in this patch.

target_read_memory is already always implemented by the target.

With gdbserver <library-list-svr4/> protocol the build-id itself seems to me
to be the natural way how to identify the library. While it could also send
a 32-bit CRC in the XML protocol the build-id looks as more universal even for
other possible GDB extensions in the future.

And thus optimizing local solib-svr4.c usage by 16 bytes saved by the 32-bit
CRC seems off-topic to me, (1) it will work needlessly differently in both
cases (vs. gdbserver) and (2) non-gdbserver usage is going to be deprecated so
this is just a temporary code anyway.

Besides that target_verify_memory fallback should be put into
default_verify_memory function and installed in to_verify_memory in all
targets except that remote.c remote_verify_memory. target_* functions should
be just dispatchers, not the implementations. (Yes, C++ would be easier.)

> I find that l_addr_inferior

l_addr should be used, not l_addr_inferior. (Although one should not use
rather either.)

> remains to be 0 if prelinked object was
> not relocated. I haven't looked at gnu ld, but I would expect it's
> missing to set it correctly (this is misfortunate).

This behavior is correct. Changing it would break all the tools around.
l_addr just has wrong comment in glibc. I have pinged now the fix:
[patch] Fix l_addr comment
http://sourceware.org/ml/libc-alpha/2011-09/msg00151.html

> >iterate so->sections..so->sections_end which contains relocated ADDR (=target
> >VMA). Then you can drop the svr4_unrelocated_vma and other calculations
> >around.
>
> Ok. I think this is what I tried first but then some testcases would
> fail. Will revisit.

There may be other issues but I am not aware of those.

Thanks,
Jan