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! > > 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