-int kho_add_subtree(struct kho_serialization *ser, const char *name, void *fdt) +int kho_add_subtree(const char *name, void *fdt) {
int err = 0;u64 phys = (u64)virt_to_phys(fdt);void *root = page_to_virt(ser->fdt);
struct kho_sub_fdt *sub_fdt;int err;
err |= fdt_begin_node(root, name);err |= fdt_property(root, PROP_SUB_FDT, &phys, sizeof(phys));err |= fdt_end_node(root);
sub_fdt = kmalloc(sizeof(*sub_fdt), GFP_KERNEL);if (!sub_fdt)return -ENOMEM;
if (err)return err;
INIT_LIST_HEAD(&sub_fdt->l);sub_fdt->name = name;sub_fdt->fdt = fdt;
return kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false);
mutex_lock(&kho_out.fdts_lock);list_add_tail(&sub_fdt->l, &kho_out.sub_fdts);err = kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false);I think you should remove sub_fdt from the list and kfree() it on error here. Otherwise we signal an error to the caller and they might free sub_fdt->fdt, which will later result in a use-after-free at __kho_finalize().
I think, it is better to simply do: WARN_ON_ONCE(kho_debugfs_fdt_add(...)); Now debugfs is optional, and there is no reason to return an error to a caller if kho_debugfs_fdt_add() fails
mutex_unlock(&kho_out.fdts_lock);return err;} EXPORT_SYMBOL_GPL(kho_add_subtree);
-int register_kho_notifier(struct notifier_block *nb) +void kho_remove_subtree(void *fdt) {
return blocking_notifier_chain_register(&kho_out.chain_head, nb);-} -EXPORT_SYMBOL_GPL(register_kho_notifier);
struct kho_sub_fdt *sub_fdt;mutex_lock(&kho_out.fdts_lock);list_for_each_entry(sub_fdt, &kho_out.sub_fdts, l) {list_for_each_entry_safe() here since we delete.
Not needed, we are breaking from the iterator when deleting.
bool kho_finalized(void) @@ -1232,15 +1248,17 @@ static __init int kho_init(void) { int err = 0; const void *fdt = kho_get_fdt();
struct page *fdt_page; if (!kho_enable) return 0;
kho_out.ser.fdt = alloc_page(GFP_KERNEL);if (!kho_out.ser.fdt) {
fdt_page = alloc_page(GFP_KERNEL);if (!fdt_page) { err = -ENOMEM; goto err_free_scratch; }kho_out.fdt = page_to_virt(fdt_page); err = kho_debugfs_init(); if (err)@@ -1268,8 +1286,8 @@ static __init int kho_init(void) return 0;
err_free_fdt:
put_page(kho_out.ser.fdt);kho_out.ser.fdt = NULL;
put_page(fdt_page);kho_out.fdt = NULL;err_free_scratch: for (int i = 0; i < kho_scratch_cnt; i++) { void *start = __va(kho_scratch[i].addr); @@ -1280,7 +1298,7 @@ static __init int kho_init(void) kho_enable = false; return err; } -late_initcall(kho_init); +fs_initcall(kho_init);
Is this change related to this patch? Also, why fs_initcall?
static void __init kho_release_scratch(void) { @@ -1416,7 +1434,7 @@ int kho_fill_kimage(struct kimage *image) if (!kho_out.finalized) return 0;
image->kho.fdt = page_to_phys(kho_out.ser.fdt);
image->kho.fdt = virt_to_phys(kho_out.fdt); scratch_size = sizeof(*kho_scratch) * kho_scratch_cnt; scratch = (struct kexec_buf){diff --git a/kernel/kexec_handover_debugfs.c b/kernel/kexec_handover_debugfs.c index a91b279f1b23..46e9e6c0791f 100644 --- a/kernel/kexec_handover_debugfs.c +++ b/kernel/kexec_handover_debugfs.c @@ -61,14 +61,17 @@ int kho_debugfs_fdt_add(struct kho_debugfs *dbg, const char *name, return __kho_debugfs_fdt_add(&dbg->fdt_list, dir, name, fdt); }
-void kho_debugfs_cleanup(struct kho_debugfs *dbg) +void kho_debugfs_fdt_remove(struct kho_debugfs *dbg, void *fdt) {
struct fdt_debugfs *ff, *tmp;list_for_each_entry_safe(ff, tmp, &dbg->fdt_list, list) {debugfs_remove(ff->file);list_del(&ff->list);kfree(ff);
struct fdt_debugfs *ff;list_for_each_entry(ff, &dbg->fdt_list, list) {list_for_each_entry_safe() here too.
Not needed, we are breaking out on delete/free.
static int kho_test_save_data(struct kho_test_state *state, void *fdt) { phys_addr_t *folios_info __free(kvfree) = NULL; @@ -102,6 +86,9 @@ static int kho_test_save_data(struct kho_test_state *state, void *fdt) if (!err) state->folios_info = no_free_ptr(folios_info);
if (!err)err = kho_test();This name is very undescriptive. Also, not the right place to add the subtree since the FDT isn't finished yet. I think it should be done from kho_test_save() instead. This patch is also missing removing the subtree at exit, and that can cause a UAF.
I sent you a patch earlier with my take on how it should be done. I still think that is the way to go: https://lore.kernel.org/all/mafs0347woui2.fsf@kernel.org/
Sure, I updated to use your suggested changes.