linux-kernel - Re: [PATCH] recordmcount: support >64k sections

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]

Message-ID: <20200424191832.GA231432@google.com>
Date:   Fri, 24 Apr 2020 12:18:32 -0700
From:   Sami Tolvanen <samitolvanen@...gle.com>
To:     Matt Helsley <mhelsley@...are.com>,
        "Steven Rostedt (VMware)" <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
        Kees Cook <keescook@...omium.org>,
        linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] recordmcount: support >64k sections

Hi Matt,

On Thu, Apr 23, 2020 at 02:47:34PM -0700, Matt Helsley wrote:
> > +static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)
> 
> I noticed this returns an unsigned int ...
> 
> > +	Elf_Shdr *const shdr0 = (Elf_Shdr *)(old_shoff + (void *)ehdr);
> > +	unsigned const old_shnum = get_shnum(ehdr, shdr0);
> 
> While this is not explicitly called out as an unsigned int. Perhaps we
> could just make this and new_shnum explicit unsigned ints and then...

> > +	if (!ehdr->e_shnum || new_shnum >= SHN_LORESERVE) {
> > +		ehdr->e_shnum = 0;
> > +		shdr0->sh_size = w(new_shnum);
> > +	} else
> > +		ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum));
> 
> If we make the unsigned int change proposed above can we reuse new_shnum
> here like so:
> 		ehdr->e_shnum = w2(new_shnum);
> 
> So this if/else is doing the inverse of get_shnum(). I think the code
> could be cleaned up a little and prepare for moving to objtool by
> putting it in a helper function.

Sure, sounds good to me.

> > +	for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
> > +		if (relhdr->sh_type == SHT_SYMTAB)
> > +			symtab = (void *)ehdr + relhdr->sh_offset;
> > +		else if (relhdr->sh_type == SHT_SYMTAB_SHNDX)
> > +			symtab_shndx = (void *)ehdr + relhdr->sh_offset;
> > +
> > +		if (symtab && symtab_shndx)
> > +			break;
> > +	}
> 
> Could you break this out into a helper function? find_symtab() maybe? Again, I think
> that helper would go away with conversion to objtool.

Agreed, this wouldn't be needed with libelf. I'll send v2 shortly.
Thanks for the review!

Sami