On Mon, Oct 28, 2024 at 5:06 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Sat, Oct 26, 2024 at 12:54 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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!
I think https://lore.kernel.org/all/20241101000017.3856204-1-surenb@xxxxxxxxxx/
should fix these warnings and avoid NULL dereference.
Thanks,
Suren.
>
> >
> > 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
