linux-kernel - Re: [RFC][PATCH 1/3] objtool: Clean up elf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 15 Jun 2020 11:34:48 -0700
From:   Matt Helsley <mhelsley@...are.com>
To:     Peter Zijlstra <peterz@...radead.org>
CC:     Josh Poimboeuf <jpoimboe@...hat.com>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
        <dvyukov@...gle.com>, <elver@...gle.com>, <andreyknvl@...gle.com>,
        <mark.rutland@....com>, <rostedt@...dmis.org>,
        <jthierry@...hat.com>, <mbenes@...e.cz>
Subject: Re: [RFC][PATCH 1/3] objtool: Clean up elf_write() condition

On Fri, Jun 12, 2020 at 04:30:35PM +0200, Peter Zijlstra wrote:
> With there being multiple ways to change the ELF data, let's more
> concisely track modification.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>

Would it make sense to set the relocation section's "changed" flag in
addition to the elf struct's changed flag in elf_rebuild_reloc_section()?

Right now I think the code is  assuming that it's a newly created section
but it would be more defensive to set it during a rebuild too I think.

Otherwise looks good to me.

> ---
>  tools/objtool/check.c |    2 ++
>  tools/objtool/elf.c   |    8 +++++++-
>  tools/objtool/elf.h   |    3 ++-
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2779,7 +2779,9 @@ int check(const char *_objname, bool orc
>  		ret = create_orc_sections(&file);
>  		if (ret < 0)
>  			goto out;
> +	}
>  
> +	if (file.elf->changed) {
>  		ret = elf_write(file.elf);
>  		if (ret < 0)
>  			goto out;
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -746,6 +746,8 @@ struct section *elf_create_section(struc
>  	elf_hash_add(elf->section_hash, &sec->hash, sec->idx);
>  	elf_hash_add(elf->section_name_hash, &sec->name_hash, str_hash(sec->name));
>  
> +	elf->changed = true;
> +
>  	return sec;
>  }
>  
> @@ -862,7 +864,7 @@ int elf_rebuild_reloc_section(struct sec
>  	return elf_rebuild_rela_section(sec, nr);
>  }
>  
> -int elf_write(const struct elf *elf)
> +int elf_write(struct elf *elf)
>  {
>  	struct section *sec;
>  	Elf_Scn *s;
> @@ -879,6 +881,8 @@ int elf_write(const struct elf *elf)
>  				WARN_ELF("gelf_update_shdr");
>  				return -1;
>  			}
> +
> +			sec->changed = false;
>  		}
>  	}
>  
> @@ -891,6 +895,8 @@ int elf_write(const struct elf *elf)
>  		return -1;
>  	}
>  
> +	elf->changed = false;
> +
>  	return 0;
>  }
>  
> --- a/tools/objtool/elf.h
> +++ b/tools/objtool/elf.h
> @@ -79,6 +79,7 @@ struct elf {
>  	Elf *elf;
>  	GElf_Ehdr ehdr;
>  	int fd;
> +	bool changed;
>  	char *name;
>  	struct list_head sections;
>  	DECLARE_HASHTABLE(symbol_hash, ELF_HASH_BITS);
> @@ -121,7 +122,7 @@ struct elf *elf_open_read(const char *na
>  struct section *elf_create_section(struct elf *elf, const char *name, size_t entsize, int nr);
>  struct section *elf_create_reloc_section(struct elf *elf, struct section *base, int reltype);
>  void elf_add_reloc(struct elf *elf, struct reloc *reloc);
> -int elf_write(const struct elf *elf);
> +int elf_write(struct elf *elf);
>  void elf_close(struct elf *elf);
>  
>  struct section *find_section_by_name(const struct elf *elf, const char *name);
> 
>