From 375bfca3459db1c5596c28c56d2ebac26e51c7b3 Mon Sep 17 00:00:00 2001 From: Alice Ferrazzi Date: Tue, 5 Feb 2019 03:33:28 +0900 Subject: [PATCH 1/6] livepatch: core: Return EOPNOTSUPP instead of ENOSYS As a result of an unsupported operation is better to use EOPNOTSUPP as error code. ENOSYS is only used for 'invalid syscall nr' and nothing else. Signed-off-by: Alice Ferrazzi Acked-by: Miroslav Benes Acked-by: Josh Poimboeuf Signed-off-by: Petr Mladek --- kernel/livepatch/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index adca5cf07f7e..5fc98a1cc3c3 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -1036,7 +1036,7 @@ int klp_enable_patch(struct klp_patch *patch) if (!klp_have_reliable_stack()) { pr_err("This architecture doesn't have support for the livepatch consistency model.\n"); - return -ENOSYS; + return -EOPNOTSUPP; } From ecba29f434a8fa333356d54d2491d174c4aab8de Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Mon, 4 Feb 2019 14:56:50 +0100 Subject: [PATCH 2/6] livepatch: Introduce klp_for_each_patch macro There are already macros to iterate over struct klp_func and klp_object. Add also klp_for_each_patch(). But make it internal because also klp_patches list is internal. Suggested-by: Josh Poimboeuf Acked-by: Miroslav Benes Acked-by: Joe Lawrence Signed-off-by: Petr Mladek --- kernel/livepatch/core.c | 8 ++++---- kernel/livepatch/core.h | 6 ++++++ kernel/livepatch/transition.c | 2 +- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 5fc98a1cc3c3..4b7f55d9e89c 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -554,7 +554,7 @@ static int klp_add_nops(struct klp_patch *patch) struct klp_patch *old_patch; struct klp_object *old_obj; - list_for_each_entry(old_patch, &klp_patches, list) { + klp_for_each_patch(old_patch) { klp_for_each_object(old_patch, old_obj) { int err; @@ -1089,7 +1089,7 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch) { struct klp_patch *old_patch, *tmp_patch; - list_for_each_entry_safe(old_patch, tmp_patch, &klp_patches, list) { + klp_for_each_patch_safe(old_patch, tmp_patch) { if (old_patch == new_patch) return; @@ -1133,7 +1133,7 @@ static void klp_cleanup_module_patches_limited(struct module *mod, struct klp_patch *patch; struct klp_object *obj; - list_for_each_entry(patch, &klp_patches, list) { + klp_for_each_patch(patch) { if (patch == limit) break; @@ -1180,7 +1180,7 @@ int klp_module_coming(struct module *mod) */ mod->klp_alive = true; - list_for_each_entry(patch, &klp_patches, list) { + klp_for_each_patch(patch) { klp_for_each_object(patch, obj) { if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) continue; diff --git a/kernel/livepatch/core.h b/kernel/livepatch/core.h index e6200f38701f..ec43a40b853f 100644 --- a/kernel/livepatch/core.h +++ b/kernel/livepatch/core.h @@ -7,6 +7,12 @@ extern struct mutex klp_mutex; extern struct list_head klp_patches; +#define klp_for_each_patch_safe(patch, tmp_patch) \ + list_for_each_entry_safe(patch, tmp_patch, &klp_patches, list) + +#define klp_for_each_patch(patch) \ + list_for_each_entry(patch, &klp_patches, list) + void klp_free_patch_start(struct klp_patch *patch); void klp_discard_replaced_patches(struct klp_patch *new_patch); void klp_discard_nops(struct klp_patch *new_patch); diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c index 300273819674..a3a6f32c6fd0 100644 --- a/kernel/livepatch/transition.c +++ b/kernel/livepatch/transition.c @@ -642,6 +642,6 @@ void klp_force_transition(void) for_each_possible_cpu(cpu) klp_update_patch_state(idle_task(cpu)); - list_for_each_entry(patch, &klp_patches, list) + klp_for_each_patch(patch) patch->forced = true; } From 86e43f23c17126e32820a1b37d747d06f3056570 Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Mon, 4 Feb 2019 14:56:51 +0100 Subject: [PATCH 3/6] livepatch: return -ENOMEM on ptr_id() allocation failure Fixes the following smatch warning: lib/livepatch/test_klp_shadow_vars.c:47 ptr_id() warn: returning -1 instead of -ENOMEM is sloppy Signed-off-by: Joe Lawrence Acked-by: Miroslav Benes Signed-off-by: Petr Mladek --- lib/livepatch/test_klp_shadow_vars.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index 02f892f941dc..f5441c193166 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -44,7 +44,7 @@ static int ptr_id(void *ptr) sp = kmalloc(sizeof(*sp), GFP_ATOMIC); if (!sp) - return -1; + return -ENOMEM; sp->ptr = ptr; sp->id = count++; From 49ee4dd2e753cd13d157361d4bd28b548e3d0ee7 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Mon, 4 Feb 2019 14:56:52 +0100 Subject: [PATCH 4/6] livepatch: Proper error handling in the shadow variables selftest Add proper error handling when allocating or getting shadow variables in the selftest. It prevents an invalid pointer access in some situations. It shows the good programming practice in the others. The error codes are just the best guess and specific for this particular test. In general, klp_shadow_alloc() returns NULL also when the given shadow variable has already been allocated. In addition, both klp_shadow_alloc() and klp_shadow_get_or_alloc() might fail from other reasons when the constructor fails. Note, that the error code is not really important even in the real life. The use of shadow variables should be transparent for the original livepatched code. Acked-by: Miroslav Benes Acked-by: Joe Lawrence Signed-off-by: Petr Mladek --- lib/livepatch/test_klp_shadow_vars.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index f5441c193166..fe5c413efe96 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -154,22 +154,37 @@ static int test_klp_shadow_vars_init(void) * Allocate a few shadow variables with different and . */ sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1); + if (!sv1) + return -ENOMEM; + sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2); + if (!sv2) + return -ENOMEM; + sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3); + if (!sv3) + return -ENOMEM; /* * Verify we can find our new shadow variables and that they point * to expected data. */ ret = shadow_get(obj, id); + if (!ret) + return -EINVAL; if (ret == sv1 && *sv1 == &var1) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv1), ptr_id(*sv1)); + ret = shadow_get(obj + 1, id); + if (!ret) + return -EINVAL; if (ret == sv2 && *sv2 == &var2) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv2), ptr_id(*sv2)); ret = shadow_get(obj, id + 1); + if (!ret) + return -EINVAL; if (ret == sv3 && *sv3 == &var3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3)); @@ -179,7 +194,12 @@ static int test_klp_shadow_vars_init(void) * The second invocation should return the same shadow var. */ sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4); + if (!sv4) + return -ENOMEM; + ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4); + if (!ret) + return -EINVAL; if (ret == sv4 && *sv4 == &var4) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv4), ptr_id(*sv4)); @@ -207,6 +227,8 @@ static int test_klp_shadow_vars_init(void) * We should still find an variable. */ ret = shadow_get(obj, id + 1); + if (!ret) + return -EINVAL; if (ret == sv3 && *sv3 == &var3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3)); From a087cdd4073b78f4739ce6709daeb4f3267b4dbf Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Mon, 4 Feb 2019 14:56:53 +0100 Subject: [PATCH 5/6] livepatch: Module coming and going callbacks can proceed with all listed patches Livepatches can no longer get enabled and disabled repeatedly. The list klp_patches contains only enabled patches and eventually the patch in transition. The module coming and going callbacks do no longer need to check for these state. They have to proceed with all listed patches. Suggested-by: Josh Poimboeuf Acked-by: Miroslav Benes Acked-by: Joe Lawrence Signed-off-by: Petr Mladek --- kernel/livepatch/core.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c index 4b7f55d9e89c..d1af69e9f0e3 100644 --- a/kernel/livepatch/core.c +++ b/kernel/livepatch/core.c @@ -1141,21 +1141,14 @@ static void klp_cleanup_module_patches_limited(struct module *mod, if (!klp_is_module(obj) || strcmp(obj->name, mod->name)) continue; - /* - * Only unpatch the module if the patch is enabled or - * is in transition. - */ - if (patch->enabled || patch == klp_transition_patch) { + if (patch != klp_transition_patch) + klp_pre_unpatch_callback(obj); - if (patch != klp_transition_patch) - klp_pre_unpatch_callback(obj); + pr_notice("reverting patch '%s' on unloading module '%s'\n", + patch->mod->name, obj->mod->name); + klp_unpatch_object(obj); - pr_notice("reverting patch '%s' on unloading module '%s'\n", - patch->mod->name, obj->mod->name); - klp_unpatch_object(obj); - - klp_post_unpatch_callback(obj); - } + klp_post_unpatch_callback(obj); klp_free_object_loaded(obj); break; @@ -1194,13 +1187,6 @@ int klp_module_coming(struct module *mod) goto err; } - /* - * Only patch the module if the patch is enabled or is - * in transition. - */ - if (!patch->enabled && patch != klp_transition_patch) - break; - pr_notice("applying patch '%s' to loading module '%s'\n", patch->mod->name, obj->mod->name); From fbb76d579dff4a2e332566dcd1d5979ac92bc34b Mon Sep 17 00:00:00 2001 From: Joe Lawrence Date: Fri, 8 Feb 2019 15:57:37 -0500 Subject: [PATCH 6/6] livepatch/selftests: use "$@" to preserve argument list The livepatch selftest functions.sh library uses "$*" and an intermediate variable to extract and then pass arguments from function to function call. The effect of this combination is that the argument list is flattened into a single argument. Sometimes this is benign, but in cases like __load_mod(), the modprobe invocation will interpret all the module parameters as a single parameter. Drop the intermediate variable and use the "$@" special parameter as described in the bash manual. Link: https://www.gnu.org/software/bash/manual/bash.html#Special-Parameters Signed-off-by: Joe Lawrence Reviewed-by: Kamalesh Babulal Acked-by: Miroslav Benes Signed-off-by: Petr Mladek --- .../testing/selftests/livepatch/functions.sh | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index c7b9fb45d7c9..30195449c63c 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -55,11 +55,10 @@ function is_livepatch_mod() { function __load_mod() { local mod="$1"; shift - local args="$*" - local msg="% modprobe $mod $args" + local msg="% modprobe $mod $*" log "${msg%% }" - ret=$(modprobe "$mod" "$args" 2>&1) + ret=$(modprobe "$mod" "$@" 2>&1) if [[ "$ret" != "" ]]; then die "$ret" fi @@ -75,12 +74,11 @@ function __load_mod() { # params - module parameters to pass to modprobe function load_mod() { local mod="$1"; shift - local args="$*" is_livepatch_mod "$mod" && die "use load_lp() to load the livepatch module $mod" - __load_mod "$mod" "$args" + __load_mod "$mod" "$@" } # load_lp_nowait(modname, params) - load a kernel module with a livepatch @@ -89,12 +87,11 @@ function load_mod() { # params - module parameters to pass to modprobe function load_lp_nowait() { local mod="$1"; shift - local args="$*" is_livepatch_mod "$mod" || die "module $mod is not a livepatch" - __load_mod "$mod" "$args" + __load_mod "$mod" "$@" # Wait for livepatch in sysfs ... loop_until '[[ -e "/sys/kernel/livepatch/$mod" ]]' || @@ -106,9 +103,8 @@ function load_lp_nowait() { # params - module parameters to pass to modprobe function load_lp() { local mod="$1"; shift - local args="$*" - load_lp_nowait "$mod" "$args" + load_lp_nowait "$mod" "$@" # Wait until the transition finishes ... loop_until 'grep -q '^0$' /sys/kernel/livepatch/$mod/transition' || @@ -120,11 +116,10 @@ function load_lp() { # params - module parameters to pass to modprobe function load_failing_mod() { local mod="$1"; shift - local args="$*" - local msg="% modprobe $mod $args" + local msg="% modprobe $mod $*" log "${msg%% }" - ret=$(modprobe "$mod" "$args" 2>&1) + ret=$(modprobe "$mod" "$@" 2>&1) if [[ "$ret" == "" ]]; then die "$mod unexpectedly loaded" fi