linux-kernel - Re: [PATCH v2 03/19] gendwarfksyms: Add address matching

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: <CABCJKud2za5o19O4XuE-jxuevwQLZPd+CdoB5tMHCgtG2Q2upQ@mail.gmail.com>
Date: Tue, 27 Aug 2024 21:28:42 +0000
From: Sami Tolvanen <samitolvanen@...gle.com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Matthew Maurer <mmaurer@...gle.com>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Gary Guo <gary@...yguo.net>, Neal Gompa <neal@...pa.dev>, 
	Hector Martin <marcan@...can.st>, Janne Grunau <j@...nau.net>, Asahi Linux <asahi@...ts.linux.dev>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-modules@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 03/19] gendwarfksyms: Add address matching

Hi Petr,

On Tue, Aug 27, 2024 at 12:40 PM Petr Pavlu <petr.pavlu@...e.com> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > +static inline u32 symbol_addr_hash(const struct symbol_addr *addr)
> > +{
> > +     return jhash(addr, sizeof(struct symbol_addr), 0);
>
> I would be careful and avoid including the padding between
> symbol_addr.section and symbol_addr.address in the hash calculation.

Good catch. I'll fix this in the next version.

> > +static int elf_for_each_symbol(int fd, elf_symbol_callback_t func, void *arg)
> > +{
> > +     size_t sym_size;
> > +     GElf_Shdr shdr_mem;
> > +     GElf_Shdr *shdr;
> > +     Elf_Data *xndx_data = NULL;
> > +     Elf_Scn *scn;
> > +     Elf *elf;
> > +
> > +     if (elf_version(EV_CURRENT) != EV_CURRENT) {
> > +             error("elf_version failed: %s", elf_errmsg(-1));
> > +             return -1;
> > +     }
> > +
> > +     elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> > +     if (!elf) {
> > +             error("elf_begin failed: %s", elf_errmsg(-1));
> > +             return -1;
> > +     }
> > +
> > +     sym_size = gelf_getclass(elf) == ELFCLASS32 ? sizeof(Elf32_Sym) :
> > +                                                   sizeof(Elf64_Sym);
> > +
> > +     scn = elf_nextscn(elf, NULL);
> > +
> > +     while (scn) {
> > +             shdr = gelf_getshdr(scn, &shdr_mem);
> > +
> > +             if (shdr && shdr->sh_type == SHT_SYMTAB_SHNDX) {
> > +                     xndx_data = elf_getdata(scn, NULL);
> > +                     break;
> > +             }
> > +
> > +             scn = elf_nextscn(elf, scn);
> > +     }
> > +
> > +     scn = elf_nextscn(elf, NULL);
> > +
> > +     while (scn) {
> > +             shdr = gelf_getshdr(scn, &shdr_mem);
> > +
> > +             if (shdr && shdr->sh_type == SHT_SYMTAB) {
> > +                     Elf_Data *data = elf_getdata(scn, NULL);
> > +                     unsigned int nsyms = data->d_size / sym_size;
>
> I think strictly speaking this should be:
> size_t nsyms = shdr->sh_size / shdr->sh_entsize;
> .. and the code could check that shdr->sh_entsize is same as what
> gelf_fsize(elf, ELF_T_SYM, 1, EV_CURRENT) returns.

Sure, I can change this. I'm not sure if there's a situation where the
current calculation wouldn't result in the exact same result though.

> > +                     unsigned int n;
> > +
> > +                     for (n = 0; n < nsyms; ++n) {
>
> The first symbol in the symbol table is always undefined, the loop can
> start from 1.

Ack.

> Alternatively, since elf_for_each_symbol() ends up in the entire series
> being used only with process_symbol() which skips symbols with the local
> binding, the function could be renamed to elf_for_each_global_symbol()
> and start the loop from shdr->sh_info.

Patch 15 ("Add support for declaration-only data structures") actually
also needs to process local symbols, so we can't skip them completely.

Sami