linux-kernel - Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation

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:   Wed, 14 Jun 2017 08:27:32 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Jiri Slaby <jslaby@...e.cz>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org,
        live-patching@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Lutomirski <luto@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [RFC PATCH 04/10] objtool: add undwarf debuginfo generation

On Wed, Jun 14, 2017 at 10:42:19AM +0200, Jiri Slaby wrote:
> On 06/01/2017, 07:44 AM, Josh Poimboeuf wrote:
> ...
> > index 3fb0747..1ca5d9a 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> ...
> > +int elf_write_to_file(struct elf *elf, char *outfile)
> > +{
> > +	int fd;
> > +	struct section *sec;
> > +	Elf *elfout;
> > +	GElf_Ehdr eh, ehout;
> > +	Elf_Scn *scn;
> > +	Elf_Data *data;
> > +	GElf_Shdr sh;
> > +
> > +	fd = creat(outfile, 0777);
> 
> 0755 even though it is umasked?
> 
> > +	if (fd == -1) {
> > +		perror("creat");
> > +		return -1;
> > +	}
> > +
> > +	elfout = elf_begin(fd, ELF_C_WRITE, NULL);
> > +	if (!elfout) {
> > +		perror("elf_begin");
> > +		return -1;
> > +	}
> > +
> > +	if (!gelf_newehdr(elfout, gelf_getclass(elf->elf))) {
> > +		perror("gelf_newehdr");
> > +		return -1;
> > +	}
> > +
> > +	if (!gelf_getehdr(elfout, &ehout)) {
> 
> This does not make much sense to do. You memset(0) it below.
> 
> > +		perror("gelf_getehdr");
> > +		return -1;
> > +	}
> > +
> > +	if (!gelf_getehdr(elf->elf, &eh)) {
> > +		perror("gelf_getehdr");
> > +		return -1;
> > +	}
> > +
> > +	memset(&ehout, 0, sizeof(ehout));
> > +	ehout.e_ident[EI_DATA] = eh.e_ident[EI_DATA];
> > +	ehout.e_machine = eh.e_machine;
> > +	ehout.e_type = eh.e_type;
> > +	ehout.e_version = EV_CURRENT;
> > +	ehout.e_shstrndx = find_section_by_name(elf, ".shstrtab")->idx;
> > +
> > +	list_for_each_entry(sec, &elf->sections, list) {
> > +		if (sec->idx == 0)
> > +			continue;
> > +
> > +		scn = elf_newscn(elfout);
> > +		if (!scn) {
> > +			perror("elf_newscn");
> > +			return -1;
> > +		}
> > +
> > +		data = elf_newdata(scn);
> > +		if (!data) {
> > +			perror("elf_newdata");
> > +			return -1;
> > +		}
> > +
> > +		if (!elf_flagdata(data, ELF_C_SET, ELF_F_DIRTY)) {
> > +			perror("elf_flagdata");
> > +			return -1;
> > +		}
> 
> There is not much point setting DIRTY flag here. elf_newdata does so.
> 
> > +		data->d_type = sec->data->d_type;
> > +		data->d_buf = sec->data->d_buf;
> > +		data->d_size = sec->data->d_size;
> > +
> > +		if(!gelf_getshdr(scn, &sh)) {
> > +			perror("gelf_getshdr");
> > +			return -1;
> > +		}
> 
> This does not make much sense to do again. You overwrite the content
> right away:
> 
> > +		sh = sec->sh;
> > +
> > +		if (!gelf_update_shdr(scn, &sh)) {
> > +			perror("gelf_update_shdr");
> > +			return -1;
> > +		}
> > +	}
> > +
> > +	if (!gelf_update_ehdr(elfout, &ehout)) {
> > +		perror("gelf_update_ehdr");
> > +		return -1;
> > +	}
> > +
> > +	if (elf_update(elfout, ELF_C_WRITE) < 0) {
> > +		perror("elf_update");
> > +		return -1;
> > +	}
> 
> elf_end() + close() ?
> 
> > +
> > +	return 0;
> > +}
> > +
> >  void elf_close(struct elf *elf)
> >  {
> >  	struct section *sec, *tmpsec;
> 
> ...
> 
> > --- /dev/null
> > +++ b/tools/objtool/undwarf.c
> > @@ -0,0 +1,308 @@
> ...
> > +int undwarf_dump(const char *_objname)
> > +{
> > +	struct elf *elf;
> > +	struct section *sec;
> > +	struct rela *rela;
> > +	struct undwarf *undwarf;
> > +	int nr, i;
> > +
> > +	objname = _objname;
> > +
> > +	elf = elf_open(objname);
> > +	if (!elf) {
> > +		WARN("error reading elf file %s\n", objname);
> > +		return 1;
> > +	}
> > +
> > +	sec = find_section_by_name(elf, ".undwarf");
> > +	if (!sec || !sec->rela)
> > +		return 0;
> > +
> > +	nr = sec->len / sizeof(*undwarf);
> > +	for (i = 0; i < nr; i++) {
> ...
> > +	}
> 
> elf_close() ?
> 
> > +
> > +	return 0;
> > +}

I agree with all your comments, will fix them all.  Thanks for the
review.

-- 
Josh