Re: [patch] validate binary before use

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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