Re: [musl] [PATCH] Add REL_COPY size change detection

[Rich Felker <dalias@libc.org>]

On Wed, Feb 26, 2020 at 06:24:48AM +0100, Markus Wichmann wrote:
> Hi all,
> 
> I was recently reading the Oracle docs about the ELF, and I came across
> their chapter about the COPY relocation. They discuraged its use, since
> with those relocations, a binding exists between importing and exporting
> module. If the semantics of the imported object changes, then this is an
> ABI mismatch.
> 
> So I looked at the musl source code and noticed that COPY relocations
> are simply processed, and an ABI mismatch is simply accepted. So, since
> I am of the opinion that detectable errors should be detected, rather
> than left to fester and spring a hard-to-explain bug on you, usually
> five minutes before deadline, I wrote the attached patch to add
> detection for at least a changed size. This won't detect all changes to
> ABI regarding COPY relocation (e.g.int-->float, or in an array of
> structs, a change to the struct size and to the array size cancelling
> each other out), but it should find most of them.
> 
> Also, I wondered whether COPY relocations are even still in use. But on
> my system (currently some Ubuntu version) I found over 15000 of the
> things. Mostly for stdout and stderr, though.

In theory copy relocations should have been gone for everyone who
switched to PIE, but then the gcc/binutils folks brought them back as
an "optimization" (allowing pc-rel access to external symbols). :-(

Anyway, I'm in agreement that we should avoid clobbering memory. The
old code did that already by using sym->st_size rather than
def.sym->st_size, which is why I hadn't seen this as an issue. But it
can over-read (and potentially fault) if the copy reloc has a larger
size than the definition, and if it's smaller the library might
wrongly access past the end of the copy, reading or clobbering
unrelated data.

At the very least I think we ought to catch and error on the case
where def.sym->st_size>sym->st_size, since we can't honor it and
failure to honor it can produce silent memory corruption. I'm less
sure about what to do if def.sym->st_size<sym->st-size; this case
seems safe and might be desirable not to break (I vaguely recall an
intent that it be ok), but if you think there are reasons it's
dangerous I'm ok with disallowing it too. I'm having a hard time now
thinking of a reason it would really help to support that, anyway.

> From 75e98f4e4cef2eb2b867062aebc481c3b1f66498 Mon Sep 17 00:00:00 2001
> From: Markus Wichmann <nullplan@gmx.net>
> Date: Wed, 26 Feb 2020 06:09:14 +0100
> Subject: [PATCH] Add detection for changed size of a COPY relocation.
> 
> COPY relocations create an ABI binding between importing and exporting
> module. Should anything about the object in question change, that would
> be an ABI change, and therefore incompatible. While the dynamic linker
> is not capable of detecting all changes, it can detect most of them by
> detecting a changed size between import and export. Any change is a
> problem, since the source buffer will be either overread or underread.
> In any case, if the semantics of the imported object changed, the ABI
> contract is broken, and it is better to detect this than to silently
> allow it and inexplicably crash later on.
> ---
>  ldso/dynlink.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/ldso/dynlink.c b/ldso/dynlink.c
> index afec985a..618c2cbd 100644
> --- a/ldso/dynlink.c
> +++ b/ldso/dynlink.c
> @@ -435,6 +435,15 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
>  			else *reloc_addr = (size_t)base + addend;
>  			break;
>  		case REL_COPY:
> +			if (def.sym && sym->st_size != def.sym->st_size) {
> +				error("Error relocating %s: %s: Size mismatch in COPY"
> +						" relocation (exp %lu, got %lu)",
> +						dso->name, name
> +						sym->st_size + 0ul,
> +						def.sym->st_size + 0ul);

Missing , after name, and the indent is weird (2 extra levels). I'd
either align (spaces) with the opening paren or use just one indent
level, and fewer lines.

Semantically, st_size is size_t not ulong, so I'd probably lean
towards a cast to (size_t) here with %zu.

> +				if (runtime) longjmp(*rtld_fail, 1);
> +				continue;
> +			}
>  			memcpy(reloc_addr, (void *)sym_val, sym->st_size);
>  			break;
>  		case REL_OFFSET32:
> --

Otherwise LGTM.

Rich