From b7a2862f90a78fde88ac4f130855b190339700a0 Mon Sep 17 00:00:00 2001 From: Josh Poimboeuf Date: Fri, 25 Apr 2014 23:05:26 -0500 Subject: [PATCH] safe kpatch unload Currently the patch module calls kpatch_unregister in the patch module exit path. If the activeness safety check fails in kpatch_unregister, it's too late for the patch module to stop exiting, so all it can do is panic. Prevent this scenario by requiring the user to disable the patch module via sysfs before allowing the module to be unloaded. The sysfs write will fail if the activeness safety check fails. An rmmod will fail if the patch is still enabled. Also add support for this new unloading model in "kpatch unload". --- README.md | 3 +- kmod/core/core.c | 42 ++++++++++++++++++ kmod/core/kpatch.h | 4 ++ kmod/patch/kpatch-patch-hook.c | 73 ++++++++++++++++++++++++-------- kpatch-build/link-vmlinux-syms.c | 3 +- kpatch/kpatch | 17 ++++++-- 6 files changed, 120 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 111632c..9898c0a 100644 --- a/README.md +++ b/README.md @@ -259,7 +259,8 @@ kpatch needs gcc >= 4.6 and Linux >= 3.7 for use of the -mfentry flag. **Q. Is it possible to remove a patch?** -Yes. Just unload the patch module and the original function will be restored. +Yes. Just run `kpatch unload` which will disable and unload the patch module +and restore the function to its original state. **Q. Can you apply multiple patches?** diff --git a/kmod/core/core.c b/kmod/core/core.c index 640ac06..aef71c6 100644 --- a/kmod/core/core.c +++ b/kmod/core/core.c @@ -53,6 +53,10 @@ DEFINE_SEMAPHORE(kpatch_mutex); static int kpatch_num_registered; +static struct kobject *kpatch_root_kobj; +struct kobject *kpatch_patches_kobj; +EXPORT_SYMBOL_GPL(kpatch_patches_kobj); + struct kpatch_backtrace_args { struct kpatch_module *kpmod; int ret; @@ -354,8 +358,15 @@ int kpatch_register(struct kpatch_module *kpmod) if (!kpmod->mod || !funcs || !num_funcs) return -EINVAL; + kpmod->enabled = false; + down(&kpatch_mutex); + if (!try_module_get(kpmod->mod)) { + ret = -ENODEV; + goto err_up; + } + for (i = 0; i < num_funcs; i++) { struct kpatch_func *func = &funcs[i]; @@ -418,6 +429,8 @@ int kpatch_register(struct kpatch_module *kpmod) pr_notice("loaded patch module \"%s\"\n", kpmod->mod->name); atomic_set(&kpatch_operation, KPATCH_OP_NONE); + kpmod->enabled = true; + up(&kpatch_mutex); return 0; @@ -433,6 +446,8 @@ err_unregister: kpatch_num_registered--; err_rollback: kpatch_remove_funcs_from_filter(funcs, num_funcs); + module_put(kpmod->mod); +err_up: up(&kpatch_mutex); return ret; } @@ -444,6 +459,8 @@ int kpatch_unregister(struct kpatch_module *kpmod) int num_funcs = kpmod->num_funcs; int i, ret; + WARN_ON(!kpmod->enabled); + down(&kpatch_mutex); /* Start unpatching operation */ @@ -483,6 +500,9 @@ int kpatch_unregister(struct kpatch_module *kpmod) pr_notice("unloaded patch module \"%s\"\n", kpmod->mod->name); + kpmod->enabled = false; + module_put(kpmod->mod); + out: atomic_set(&kpatch_operation, KPATCH_OP_NONE); up(&kpatch_mutex); @@ -490,4 +510,26 @@ out: } EXPORT_SYMBOL(kpatch_unregister); +static int kpatch_init(void) +{ + kpatch_root_kobj = kobject_create_and_add("kpatch", kernel_kobj); + if (!kpatch_root_kobj) + return -ENOMEM; + + kpatch_patches_kobj = kobject_create_and_add("patches", + kpatch_root_kobj); + if (!kpatch_patches_kobj) + return -ENOMEM; + + return 0; +} + +static void kpatch_exit(void) +{ + kobject_put(kpatch_patches_kobj); + kobject_put(kpatch_root_kobj); +} + +module_init(kpatch_init); +module_exit(kpatch_exit); MODULE_LICENSE("GPL"); diff --git a/kmod/core/kpatch.h b/kmod/core/kpatch.h index 9801fce..0c7115a 100644 --- a/kmod/core/kpatch.h +++ b/kmod/core/kpatch.h @@ -40,8 +40,12 @@ struct kpatch_module { struct module *mod; struct kpatch_func *funcs; int num_funcs; + + bool enabled; }; +extern struct kobject *kpatch_patches_kobj; + extern int kpatch_register(struct kpatch_module *kpmod); extern int kpatch_unregister(struct kpatch_module *kpmod); diff --git a/kmod/patch/kpatch-patch-hook.c b/kmod/patch/kpatch-patch-hook.c index a0d4a32..61c5921 100644 --- a/kmod/patch/kpatch-patch-hook.c +++ b/kmod/patch/kpatch-patch-hook.c @@ -28,6 +28,43 @@ extern char __kpatch_patches, __kpatch_patches_end; static struct kpatch_module kpmod; +static struct kobject *patch_kobj; + +static ssize_t patch_enabled_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + return sprintf(buf, "%d\n", kpmod.enabled); +} + +static ssize_t patch_enabled_store(struct kobject *kobj, + struct kobj_attribute *attr, char *buf, + size_t count) +{ + int ret; + unsigned long val; + + /* only disabling is supported */ + if (!kpmod.enabled) + return -EINVAL; + + ret = kstrtoul(buf, 10, &val); + if (ret) + return ret; + + val = !!val; + + /* only disabling is supported */ + if (val) + return -EINVAL; + + ret = kpatch_unregister(&kpmod); + if (ret) + return ret; + + return count; +} +static struct kobj_attribute patch_enabled_attr = + __ATTR(enabled, 0644, patch_enabled_show, patch_enabled_store); static int __init patch_init(void) { @@ -51,12 +88,27 @@ static int __init patch_init(void) kpmod.funcs[i].new_size = patches[i].new_size; } + patch_kobj = kobject_create_and_add(THIS_MODULE->name, + kpatch_patches_kobj); + if (!patch_kobj) { + ret = -ENOMEM; + goto err_free; + } + + ret = sysfs_create_file(patch_kobj, &patch_enabled_attr.attr); + if (ret) + goto err_put; + ret = kpatch_register(&kpmod); if (ret) - goto err_free; + goto err_sysfs; return 0; +err_sysfs: + sysfs_remove_file(patch_kobj, &patch_enabled_attr.attr); +err_put: + kobject_put(patch_kobj); err_free: kfree(kpmod.funcs); return ret; @@ -64,22 +116,9 @@ err_free: static void __exit patch_exit(void) { - int ret; - - ret = kpatch_unregister(&kpmod); - if (ret) { - /* - * TODO: If this happens, we're screwed. We need a way to - * prevent the module from unloading if the activeness safety - * check fails. - * - * Or alternatively we could keep trying the activeness safety - * check in a loop, until it works or we timeout. Then we - * could panic. - */ - panic("kpatch_unregister failed: %d", ret); - } - + WARN_ON(kpmod.enabled); + sysfs_remove_file(patch_kobj, &patch_enabled_attr.attr); + kobject_put(patch_kobj); kfree(kpmod.funcs); } diff --git a/kpatch-build/link-vmlinux-syms.c b/kpatch-build/link-vmlinux-syms.c index 01679a1..af0855b 100644 --- a/kpatch-build/link-vmlinux-syms.c +++ b/kpatch-build/link-vmlinux-syms.c @@ -237,7 +237,8 @@ int main(int argc, char **argv) GELF_ST_BIND(cur->sym.st_info) != STB_GLOBAL || cur->sym.st_shndx != STN_UNDEF || !strcmp(cur->name, "kpatch_register") || - !strcmp(cur->name, "kpatch_unregister")) + !strcmp(cur->name, "kpatch_unregister") || + !strcmp(cur->name, "kpatch_patches_kobj")) continue; printf("found global symbol %s\n", cur->name); diff --git a/kpatch/kpatch b/kpatch/kpatch index ce3dc04..1612269 100755 --- a/kpatch/kpatch +++ b/kpatch/kpatch @@ -100,6 +100,19 @@ load_module () { } unload_module () { + # strip leading path, .ko suffix and convert '-' to '_' so we can find + # the module name in sysfs + PATCH="$(basename $1)" + PATCH="${PATCH%.ko}" + PATCH="${PATCH//-/_}" + + ENABLED=/sys/kernel/kpatch/patches/"$PATCH"/enabled + + if [[ -e "$ENABLED" ]] && [[ $(cat "$ENABLED") -eq 1 ]]; then + echo "disabling patch module: $1" + echo 0 > $ENABLED || die "can't disable $PATCH" + fi + echo "unloading patch module: $1" /usr/sbin/rmmod "$(basename $1)" } @@ -135,9 +148,7 @@ case "$1" in "unload") [[ "$#" -ne 2 ]] && usage - PATCH="$2" - find_module "$PATCH" || die "can't find $PATCH" - unload_module "$MODULE" || die "failed to unload patch $PATCH" + unload_module "$2" || die "failed to unload patch $2" ;; "install")