[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/s390x/arch_dump: Fixes for the name field in the PT_N
From: |
Christian Borntraeger |
Subject: |
Re: [PATCH] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section |
Date: |
Thu, 4 Feb 2021 14:01:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 |
On 03.02.21 10:44, Thomas Huth wrote:
> According to the "ELF-64 Object File Format" specification:
>
> "The first word in the entry, namesz, identifies the length, in
> bytes, of a name identifying the entry’s owner or originator. The name field
> contains a null-terminated string, with padding as necessary to ensure 8-
> byte alignment for the descriptor field. The length does not include the
> terminating null or the padding."
>
> So we should not include the terminating NUL in the length field here.
>
> Also there is a compiler warning with GCC 9.3 when compiling with
> the -fsanitize=thread compiler flag:
>
> In function 'strncpy',
> inlined from 's390x_write_elf64_notes' at
> ../target/s390x/arch_dump.c:219:9:
> /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
> '__builtin_strncpy' specified bound 8 equals destination size
> [-Werror=stringop-truncation]
>
> Since the name should always be NUL-terminated, we can simply decrease
> the size of the strncpy by one here to silence this warning. And while
> we're at it, also add an assert() to make sure that the provided names
> always fit the size field (which is fine for the current callers, the
> function is called once with "CORE" and once with "LINUX" as a name).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> The ELF-64 spec can be found here, for example:
> https://uclibc.org/docs/elf-64-gen.pdf
>
> Here's a CI run with the compiler warning:
> https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248
>
> target/s390x/arch_dump.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 50fa0ae4b6..20c3a09707 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -212,11 +212,13 @@ static int s390x_write_elf64_notes(const char
> *note_name,
> int note_size;
> int ret = -1;
>
> + assert(strlen(note_name) < sizeof(note.name));
> +
> for (nf = funcs; nf->note_contents_func; nf++) {
> memset(¬e, 0, sizeof(note));
> - note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> + note.hdr.n_namesz = cpu_to_be32(strlen(note_name));
> note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
> - strncpy(note.name, note_name, sizeof(note.name));
> + strncpy(note.name, note_name, sizeof(note.name) - 1);
This kind of feels wrong. With 8 bytes of note.name, we should be able to store
"Test123" + the final \0.
Now we tell strncpy to stop at 7. Which means that for Test123 we would NOT
copy the final \0.
So I actually think that the sanitize warning is wrong (as long as the
assertion holds true).