Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] The ELF loader lacks bounds checking for program headers and segment data against the loaded firmware size, allowing out-of-bounds memory reads. - [High] Missing validation that `p_memsz >= p_filesz` leads to integer underflow and massive out-of-bounds `memset`. - [High] Signed-to-unsigned integer promotion bypasses the spatial boundary check, allowing out-of-bounds memory writes. - [High] The ELF load alignment is passed directly to `PTR_ALIGN` without verifying it is a power of two, breaking pointer arithmetic. - [High] The bounds check uses a hardcoded maximum size instead of the actual reserved memory resource size, allowing out-of-bounds writes. - [High] Integer overflow in the calculation of the ELF binary memory footprint bypasses spatial bounds checks. - [Medium] Incorrect use of the `PTR_ALIGN` macro miscalculates alignment by unnecessarily pre-adding the alignment value, violating generic alignment semantics. --
commit a2d9d89a62300f04ed251e2db601b303f521e71c Author: Thara Gopinath <[email protected]> drivers: hv: hv_vsm_boot: load secure kernel image from firmware This commit adds a VTL0-side loader for LVBS bring-up that stages a secure kernel image into the reserved memory region. It fetches a 64-bit ELF using request_firmware(), validates the ELF header, and stages the PT_LOAD segments into memory. > diff --git a/drivers/hv/hv_vsm_boot.c b/drivers/hv/hv_vsm_boot.c > --- a/drivers/hv/hv_vsm_boot.c > +++ b/drivers/hv/hv_vsm_boot.c [ ... ] > @@ -38,6 +55,167 @@ static int hv_vsm_get_register(u32 reg_name, u64 *result) [ ... ] > +static size_t __init hv_vsm_elf_binary_size(void *image) > +{ [ ... ] > + for (i = 0; i < ehdr->e_phnum; i++, phdr++) { [Severity: High] Is it safe to blindly iterate over and read memory from phdr here? If the program header table offset or count extends beyond the firmware limits, this could result in out-of-bounds memory reads. > + if (phdr->p_type != PT_LOAD) > + continue; > + > + max_paddr = max(max_paddr, phdr->p_paddr + phdr->p_memsz); [Severity: High] Can this calculation overflow? If a malformed ELF segment defines p_paddr and p_memsz such that their sum exceeds the 64-bit integer limit, it could wrap around. This would result in an undersized binary size, potentially bypassing later spatial bounds checks and allowing out-of-bounds writes. > + } > + > + return max_paddr - min_paddr; > +} > + > +static int __init hv_vsm_load_elf(void *image, Elf64_Addr *sk_entry_pa) > +{ [ ... ] > + /* Align the base load address up to the first PT_LOAD segment > alignment */ > + base_addr = PTR_ALIGN(vsm_skm_va + first_load_align, first_load_align); [Severity: Medium] Does this code miscalculate the alignment? By explicitly adding first_load_align before invoking PTR_ALIGN, it forces the pointer to skip an entire alignment block even if it is already aligned, which wastes memory within the strictly reserved region. [Severity: High] Additionally, PTR_ALIGN relies on the alignment being a power of two. Since first_load_align is only validated below using modulo (modulo SZ_2M), could a non-power-of-two alignment corrupt the pointer calculation entirely? > + > + size = hv_vsm_elf_binary_size(image); > + if (vsm_skm_va + VSM_SK_INITIAL_MAP_SIZE - base_addr < size) { [Severity: High] Can a malicious base_addr bypass this validation entirely? If base_addr is located beyond the vsm_skm_va + VSM_SK_INITIAL_MAP_SIZE limit, the pointer arithmetic yields a negative ptrdiff_t. When compared against the unsigned size_t size, it undergoes standard C integer promotion to a massive positive value, completely bypassing the boundary condition. [Severity: High] Also, does this check artificially limit the copy size against a hardcoded constant rather than the actual bounds of the allocated sk_res region? If a user reserves a secure kernel region smaller than 16MB via the kernel command line, this could allow writes outside the dynamically allocated physical region. > + pr_err("secure kernel does not fit: %zu > %td\n", size, > + vsm_skm_va + VSM_SK_INITIAL_MAP_SIZE - base_addr); > + return -EFBIG; > + } [ ... ] > + for (i = 0; i < ehdr->e_phnum; i++, phdr++) { > + void *load_addr; > + > + if (phdr->p_type != PT_LOAD) > + continue; > + > + /* > + * Adjust the load address by min_paddr to compensate the > + * offset. > + */ > + load_addr = base_addr + (phdr->p_paddr - min_paddr); > + > + pr_debug(" p_offset: %#016llx, p_filesz: %#016llx, p_memsz: > %#016llx to pa %#016llx\n", > + phdr->p_offset, phdr->p_filesz, phdr->p_memsz, > + virt_to_phys(load_addr)); > + memcpy(load_addr, image + phdr->p_offset, phdr->p_filesz); [Severity: High] Are the ELF segment properties validated before this memcpy? The source memory is derived from phdr->p_offset and phdr->p_filesz, which don't appear to be checked against the total firmware size. This risks out-of-bounds memory reads. > + > + if (phdr->p_memsz == phdr->p_filesz) > + continue; > + > + pr_debug(" zeroing %#016llx bytes at pa %#016llx\n", > + phdr->p_memsz - phdr->p_filesz, > + virt_to_phys(load_addr + phdr->p_filesz)); > + memset(load_addr + phdr->p_filesz, 0, > + phdr->p_memsz - phdr->p_filesz); [Severity: High] Can this subtraction underflow? If a malformed ELF provides a PT_LOAD segment where phdr->p_filesz is larger than phdr->p_memsz, this calculation would result in a massive out-of-bounds memset that could destroy memory. > + } > + > + *sk_entry_pa = virt_to_phys(base_addr + (ehdr->e_entry - min_paddr)); > + pr_debug("secure kernel entry pa: %#llx\n", *sk_entry_pa); > + > + return 0; > +} > + > +static int __init hv_vsm_load_secure_kernel(Elf64_Addr *sk_entry_pa) > +{ > + const struct firmware *fw; > + Elf64_Ehdr *ehdr; > + int ret; > + > + ret = request_firmware(&fw, SK_FW_NAME, NULL); > + if (ret) { > + pr_err("Failed to load %s firmware: %d\n", SK_FW_NAME, ret); > + return ret; > + } > + > + ehdr = (Elf64_Ehdr *)fw->data; > + if (fw->size < sizeof(*ehdr) || [Severity: High] Does this bounds check sufficiently validate the firmware size? It appears to omit validation for the program headers' offset and count, which could allow a malformed firmware file to trigger out-of-bounds reads later in the loader. > + memcmp(ehdr->e_ident, ELFMAG, SELFMAG) || > + (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN)) { -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
