On Sat, Oct 26, 2024 at 12:54 AM Dan Carpenter <[email protected]> wrote:
>
> Hello Suren Baghdasaryan,
>
> Commit e88dfe467aa7 ("alloc_tag: load module tags into separate
> contiguous memory") from Oct 23, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> kernel/module/main.c:2594 move_module()
> warn: 'dest' isn't an ERR_PTR
>
> kernel/module/main.c
> 2554 static int move_module(struct module *mod, struct load_info *info)
> 2555 {
> 2556 int i;
> 2557 enum mod_mem_type t = 0;
> 2558 int ret = -ENOMEM;
> 2559 bool codetag_section_found = false;
> 2560
> 2561 for_each_mod_mem_type(type) {
> 2562 if (!mod->mem[type].size) {
> 2563 mod->mem[type].base = NULL;
> 2564 mod->mem[type].rw_copy = NULL;
> 2565 continue;
> 2566 }
> 2567
> 2568 ret = module_memory_alloc(mod, type);
> 2569 if (ret) {
> 2570 t = type;
> 2571 goto out_err;
> 2572 }
> 2573 }
> 2574
> 2575 /* Transfer each section which specifies SHF_ALLOC */
> 2576 pr_debug("Final section addresses for %s:\n", mod->name);
> 2577 for (i = 0; i < info->hdr->e_shnum; i++) {
> 2578 void *dest;
> 2579 Elf_Shdr *shdr = &info->sechdrs[i];
> 2580 const char *sname;
> 2581 unsigned long addr;
> 2582
> 2583 if (!(shdr->sh_flags & SHF_ALLOC))
> 2584 continue;
> 2585
> 2586 sname = info->secstrings + shdr->sh_name;
> 2587 /*
> 2588 * Load codetag sections separately as they might
> still be used
> 2589 * after module unload.
> 2590 */
> 2591 if (codetag_needs_module_section(mod, sname,
> shdr->sh_size)) {
> 2592 dest = codetag_alloc_module_section(mod,
> sname, shdr->sh_size,
> 2593
> arch_mod_section_prepend(mod, i), shdr->sh_addralign);
> --> 2594 if (IS_ERR(dest)) {
> 2595 ret = PTR_ERR(dest);
> 2596 goto out_err;
> 2597 }
>
> This is a false positive caused my specific kernel config. The
> codetag_alloc_module_section() function returns both error pointers and NULL.
> It can return NULL because of the .config, because the section isn't found,
> because the size is too small or because the desc.alloc_section_mem() function
> isn't implemented (which is a bug).
Thanks for the report, Dan!
Yes, codetag_alloc_module_section() should return NULL if the
"alloc_tags" section is missing or empty. That's not an invalid case
and it happens when a module has no allocations.
If desc.alloc_section_mem() is not implemented that indeed should be a
different error because we should fail to load. Today I have a
WARN_ON() but that's not enough. I'll change it to return an actual
error.
>
> 2598 addr = (unsigned long)dest;
> 2599 codetag_section_found = true;
> 2600 } else {
> 2601 enum mod_mem_type type = shdr->sh_entsize >>
> SH_ENTSIZE_TYPE_SHIFT;
> 2602 unsigned long offset = shdr->sh_entsize &
> SH_ENTSIZE_OFFSET_MASK;
> 2603
> 2604 addr = (unsigned long)mod->mem[type].base +
> offset;
> 2605 dest = mod->mem[type].rw_copy + offset;
> 2606 }
> 2607
> 2608 if (shdr->sh_type != SHT_NOBITS) {
> 2609 /*
> 2610 * Our ELF checker already validated this,
> but let's
> 2611 * be pedantic and make the goal clearer. We
> actually
> 2612 * end up copying over all modifications
> made to the
> 2613 * userspace copy of the entire struct
> module.
> 2614 */
> 2615 if (i == info->index.mod &&
> 2616 (WARN_ON_ONCE(shdr->sh_size !=
> sizeof(struct module)))) {
>
> This handles the case where the size is too small. Why not return an error
> pointer if the section is not found or if there is a bug. We could also add a
> NULL check here.
I'm trying to treat the missing "alloc_tags" section as a valid case
but yeah, I'm missing some checks as you noticed below.
>
> 2617 ret = -ENOEXEC;
> 2618 goto out_err;
> 2619 }
> 2620 memcpy(dest, (void *)shdr->sh_addr,
> shdr->sh_size);
> ^^^^^
> Dereferenced without testing.
I see. dest can be NULL here. I think I'll just need to correct the
handling for dest=NULL case for tags section. I'll work on fixing that
in the next version. Thanks!
>
> 2621 }
> 2622 /*
> 2623 * Update the userspace copy's ELF section address
> to point to
> 2624 * our newly allocated memory as a pure convenience
> so that
> 2625 * users of info can keep taking advantage and using
> the newly
> 2626 * minted official memory area.
> 2627 */
> 2628 shdr->sh_addr = addr;
> 2629 pr_debug("\t0x%lx 0x%.8lx %s\n", (long)shdr->sh_addr,
> 2630 (long)shdr->sh_size, info->secstrings +
> shdr->sh_name);
> 2631 }
> 2632
> 2633 return 0;
> 2634 out_err:
> 2635 for (t--; t >= 0; t--)
> 2636 module_memory_free(mod, t);
> 2637 if (codetag_section_found)
> 2638 codetag_free_module_sections(mod);
> 2639
> 2640 return ret;
> 2641 }
>
> regards,
> dan carpenter
