[bug report] alloc_tag: load module tags into separate contiguous memory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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).

    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.

    2617                                 ret = -ENOEXEC;
    2618                                 goto out_err;
    2619                         }
    2620                         memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
                                        ^^^^^
Dereferenced without testing.

    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




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux