Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Severe out-of-bounds memory accesses and ABI incompatibility due to incorrect node sizing on 32-bit platforms. - [High] Pre-existing memory corruption risk due to `kzalloc` crossing page boundaries for KHO radix root node. - [High] Undefined behavior and logic failure due to 64-bit shift assumptions on 32-bit platforms. -- commit a4a688bed2326d7eaaf8a3a47636fe600e228c89 Author: Pratyush Yadav (Google) <[email protected]> kho: add callback for table pages The KHO memory preservation radix tree does not mark the table pages themselves as scratch. This means any walker looking for free ranges of memory outside of scratch areas will ignore the table. This patch adds a table callback that is invoked for each table page to address this. > diff --git a/include/linux/kho_radix_tree.h b/include/linux/kho_radix_tree.h > index 030da6399d288..fe7151d89361d 100644 > --- a/include/linux/kho_radix_tree.h > +++ b/include/linux/kho_radix_tree.h > @@ -37,12 +37,15 @@ struct kho_radix_tree { > /** > * struct kho_radix_walk_cb - Callbacks for KHO radix tree walk. > * @key: Called on each present key in the radix tree. > + * @table: Called on each table of the radix tree itself. Receives the > + * physical address of the page containing the table. > * > * For each callback, a return value of 0 continues the walk and a non-zero > * return value is directly returned to the caller. > */ > struct kho_radix_walk_cb { > int (*key)(unsigned long key); [Severity: High] This isn't a bug introduced by this patch, but does the key encoding work correctly on 32-bit platforms? Looking at kho_encode_radix_key(), the code performs 1UL << (KHO_ORDER_0_LOG2 - order). With KHO_ORDER_0_LOG2 defined as 64 - PAGE_SHIFT (52 for 4KB pages), doesn't shifting a 32-bit 1UL by 52 bits invoke undefined behavior? Additionally, since the radix key is typed as unsigned long (which is 32 bits on these platforms), is it structurally possible to encode and decode the 52-bit shifted physical address and order bit as designed? > + int (*table)(phys_addr_t phys); > }; > [ ... ] > diff --git a/kernel/liveupdate/kexec_handover.c > b/kernel/liveupdate/kexec_handover.c > index b22b3cec251ee..0f8d058f1a272 100644 > --- a/kernel/liveupdate/kexec_handover.c > +++ b/kernel/liveupdate/kexec_handover.c [ ... ] > @@ -295,6 +301,12 @@ static int __kho_radix_walk_tree(struct kho_radix_node > *root, > unsigned int shift; > int err; > > + if (cb->table) { > + err = cb->table(virt_to_phys(root)); [Severity: High] This isn't a bug introduced by this patch, but does the KHO radix tree root node get allocated via kzalloc(PAGE_SIZE, GFP_KERNEL) in kho_init()? The SLUB allocator does not guarantee page alignment for PAGE_SIZE objects when debugging features (like redzones) are enabled. If the root node crosses a physical page boundary, won't this table callback only receive the physical address of the start of the object? If a walker treats this address as a single page, could it fail to mark the second page as in-use? During kexec, couldn't this unmarked page be treated as free scratch memory and overwritten, corrupting the KHO radix tree for the incoming kernel? Note: I see this is fixed later in the series in commit d207a159c7c112b64706fe2fb64399a65932fdb6 ("kho: add kho_radix_init_tree()"). > + if (err) > + return err; > + } > + > for (i = 0; i < PAGE_SIZE / sizeof(phys_addr_t); i++) { [Severity: Critical] This is a pre-existing issue, but does this loop correctly bound memory accesses on 32-bit platforms? Since sizeof(phys_addr_t) is 4 bytes on 32-bit platforms, PAGE_SIZE / sizeof(phys_addr_t) evaluates to 1024 for 4KB pages. KHO_TABLE_SIZE_LOG2 evaluates to 10, meaning the table array in struct kho_radix_node has 1024 elements of type u64 (8 bytes each). This makes the structure 8192 bytes large. If node allocations only allocate a single PAGE_SIZE (4096 bytes), won't accessing root->table[i] in this loop up to 1024 read and write well past the bounds of the allocated page? Also, by relying on sizeof(phys_addr_t), doesn't the FDT format vary by platform and break ABI cross-compatibility? > if (!root->table[i]) > continue; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
