From 8f6b88662cacb1d01398c1e8be52aeac433189f6 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Thu, 16 Jan 2020 16:31:42 +0100 Subject: livepatch/sample: Use the right type for the leaking data pointer The "leak" pointer, in the sample of shadow variable API, is allocated as sizeof(int). Let's help developers and static analyzers with understanding the code by using the appropriate pointer type. Reported-by: Dan Carpenter Signed-off-by: Petr Mladek Reviewed-by: Joe Lawrence Acked-by: Miroslav Benes Reviewed-by: Kamalesh Babulal Signed-off-by: Jiri Kosina --- samples/livepatch/livepatch-shadow-fix1.c | 12 ++++++------ samples/livepatch/livepatch-shadow-fix2.c | 4 ++-- samples/livepatch/livepatch-shadow-mod.c | 4 ++-- 3 files changed, 10 insertions(+), 10 deletions(-) (limited to 'samples') diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index e89ca4546114..bab12bdb753f 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -52,8 +52,8 @@ struct dummy { */ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data) { - void **shadow_leak = shadow_data; - void *leak = ctor_data; + int **shadow_leak = shadow_data; + int *leak = ctor_data; *shadow_leak = leak; return 0; @@ -62,7 +62,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data) static struct dummy *livepatch_fix1_dummy_alloc(void) { struct dummy *d; - void *leak; + int *leak; d = kzalloc(sizeof(*d), GFP_KERNEL); if (!d) @@ -76,7 +76,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void) * variable. A patched dummy_free routine can later fetch this * pointer to handle resource release. */ - leak = kzalloc(sizeof(int), GFP_KERNEL); + leak = kzalloc(sizeof(*leak), GFP_KERNEL); if (!leak) { kfree(d); return NULL; @@ -94,7 +94,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void) static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data) { void *d = obj; - void **shadow_leak = shadow_data; + int **shadow_leak = shadow_data; kfree(*shadow_leak); pr_info("%s: dummy @ %p, prevented leak @ %p\n", @@ -103,7 +103,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data) static void livepatch_fix1_dummy_free(struct dummy *d) { - void **shadow_leak; + int **shadow_leak; /* * Patch: fetch the saved SV_LEAK shadow variable, detach and diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c index 50d223b82e8b..29fe5cd42047 100644 --- a/samples/livepatch/livepatch-shadow-fix2.c +++ b/samples/livepatch/livepatch-shadow-fix2.c @@ -59,7 +59,7 @@ static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies) static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) { void *d = obj; - void **shadow_leak = shadow_data; + int **shadow_leak = shadow_data; kfree(*shadow_leak); pr_info("%s: dummy @ %p, prevented leak @ %p\n", @@ -68,7 +68,7 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data) static void livepatch_fix2_dummy_free(struct dummy *d) { - void **shadow_leak; + int **shadow_leak; int *shadow_count; /* Patch: copy the memory leak patch from the fix1 module. */ diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c index ecfe83a943a7..7e753b0d2fa6 100644 --- a/samples/livepatch/livepatch-shadow-mod.c +++ b/samples/livepatch/livepatch-shadow-mod.c @@ -95,7 +95,7 @@ struct dummy { static __used noinline struct dummy *dummy_alloc(void) { struct dummy *d; - void *leak; + int *leak; d = kzalloc(sizeof(*d), GFP_KERNEL); if (!d) @@ -105,7 +105,7 @@ static __used noinline struct dummy *dummy_alloc(void) msecs_to_jiffies(1000 * EXPIRE_PERIOD); /* Oops, forgot to save leak! */ - leak = kzalloc(sizeof(int), GFP_KERNEL); + leak = kzalloc(sizeof(*leak), GFP_KERNEL); if (!leak) { kfree(d); return NULL; -- cgit v1.2.3-59-g8ed1b From be6da98425b69388ed31b18bd2497f826116f29b Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Thu, 16 Jan 2020 16:31:44 +0100 Subject: livepatch/samples/selftest: Use klp_shadow_alloc() API correctly The commit e91c2518a5d22a ("livepatch: Initialize shadow variables safely by a custom callback") leads to the following static checker warning: samples/livepatch/livepatch-shadow-fix1.c:86 livepatch_fix1_dummy_alloc() error: 'klp_shadow_alloc()' 'leak' too small (4 vs 8) It is because klp_shadow_alloc() is used a wrong way: int *leak; shadow_leak = klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL, shadow_leak_ctor, leak); The code is supposed to store the "leak" pointer into the shadow variable. 3rd parameter correctly passes size of the data (size of pointer). But the 5th parameter is wrong. It should pass pointer to the data (pointer to the pointer) but it passes the pointer directly. It works because shadow_leak_ctor() handle "ctor_data" as the data instead of pointer to the data. But it is semantically wrong and confusing. The same problem is also in the module used by selftests. In this case, "pvX" variables are introduced. They represent the data stored in the shadow variables. Reported-by: Dan Carpenter Signed-off-by: Petr Mladek Reviewed-by: Joe Lawrence Acked-by: Miroslav Benes Reviewed-by: Kamalesh Babulal Signed-off-by: Jiri Kosina --- lib/livepatch/test_klp_shadow_vars.c | 52 ++++++++++++++++++------------- samples/livepatch/livepatch-shadow-fix1.c | 9 ++++-- 2 files changed, 36 insertions(+), 25 deletions(-) (limited to 'samples') diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index 4e94f46234e8..f0b5a1d24e55 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -73,13 +73,13 @@ static void *shadow_alloc(void *obj, unsigned long id, size_t size, gfp_t gfp_flags, klp_shadow_ctor_t ctor, void *ctor_data) { - int *var = ctor_data; + int **var = ctor_data; int **sv; sv = klp_shadow_alloc(obj, id, size, gfp_flags, ctor, var); pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n", __func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor), - ptr_id(var), ptr_id(sv)); + ptr_id(*var), ptr_id(sv)); return sv; } @@ -88,13 +88,13 @@ static void *shadow_get_or_alloc(void *obj, unsigned long id, size_t size, gfp_t gfp_flags, klp_shadow_ctor_t ctor, void *ctor_data) { - int *var = ctor_data; + int **var = ctor_data; int **sv; sv = klp_shadow_get_or_alloc(obj, id, size, gfp_flags, ctor, var); pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n", __func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor), - ptr_id(var), ptr_id(sv)); + ptr_id(*var), ptr_id(sv)); return sv; } @@ -118,11 +118,14 @@ static void shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor) static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data) { int **sv = shadow_data; - int *var = ctor_data; + int **var = ctor_data; - *sv = var; + if (!var) + return -EINVAL; + + *sv = *var; pr_info("%s: PTR%d -> PTR%d\n", - __func__, ptr_id(sv), ptr_id(var)); + __func__, ptr_id(sv), ptr_id(*var)); return 0; } @@ -139,19 +142,24 @@ static int test_klp_shadow_vars_init(void) { void *obj = THIS_MODULE; int id = 0x1234; - size_t size = sizeof(int *); gfp_t gfp_flags = GFP_KERNEL; int var1, var2, var3, var4; + int *pv1, *pv2, *pv3, *pv4; int **sv1, **sv2, **sv3, **sv4; int **sv; + pv1 = &var1; + pv2 = &var2; + pv3 = &var3; + pv4 = &var4; + ptr_id(NULL); - ptr_id(&var1); - ptr_id(&var2); - ptr_id(&var3); - ptr_id(&var4); + ptr_id(pv1); + ptr_id(pv2); + ptr_id(pv3); + ptr_id(pv4); /* * With an empty shadow variable hash table, expect not to find @@ -164,15 +172,15 @@ 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); + sv1 = shadow_alloc(obj, id, sizeof(pv1), gfp_flags, shadow_ctor, &pv1); if (!sv1) return -ENOMEM; - sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2); + sv2 = shadow_alloc(obj + 1, id, sizeof(pv2), gfp_flags, shadow_ctor, &pv2); if (!sv2) return -ENOMEM; - sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3); + sv3 = shadow_alloc(obj, id + 1, sizeof(pv3), gfp_flags, shadow_ctor, &pv3); if (!sv3) return -ENOMEM; @@ -183,20 +191,20 @@ static int test_klp_shadow_vars_init(void) sv = shadow_get(obj, id); if (!sv) return -EINVAL; - if (sv == sv1 && *sv1 == &var1) + if (sv == sv1 && *sv1 == pv1) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv1), ptr_id(*sv1)); sv = shadow_get(obj + 1, id); if (!sv) return -EINVAL; - if (sv == sv2 && *sv2 == &var2) + if (sv == sv2 && *sv2 == pv2) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv2), ptr_id(*sv2)); sv = shadow_get(obj, id + 1); if (!sv) return -EINVAL; - if (sv == sv3 && *sv3 == &var3) + if (sv == sv3 && *sv3 == pv3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3)); @@ -204,14 +212,14 @@ static int test_klp_shadow_vars_init(void) * Allocate or get a few more, this time with the same , . * The second invocation should return the same shadow var. */ - sv4 = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4); + sv4 = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4); if (!sv4) return -ENOMEM; - sv = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4); + sv = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4); if (!sv) return -EINVAL; - if (sv == sv4 && *sv4 == &var4) + if (sv == sv4 && *sv4 == pv4) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv4), ptr_id(*sv4)); @@ -240,7 +248,7 @@ static int test_klp_shadow_vars_init(void) sv = shadow_get(obj, id + 1); if (!sv) return -EINVAL; - if (sv == sv3 && *sv3 == &var3) + if (sv == sv3 && *sv3 == pv3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3)); diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index bab12bdb753f..de0363b288a7 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -53,9 +53,12 @@ struct dummy { static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data) { int **shadow_leak = shadow_data; - int *leak = ctor_data; + int **leak = ctor_data; - *shadow_leak = leak; + if (!ctor_data) + return -EINVAL; + + *shadow_leak = *leak; return 0; } @@ -83,7 +86,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void) } klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL, - shadow_leak_ctor, leak); + shadow_leak_ctor, &leak); pr_info("%s: dummy @ %p, expires @ %lx\n", __func__, d, d->jiffies_expire); -- cgit v1.2.3-59-g8ed1b From f46e49a9cc3814f3564477f0fffc00e0a2bc9e80 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Thu, 16 Jan 2020 16:31:45 +0100 Subject: livepatch: Handle allocation failure in the sample of shadow variable API klp_shadow_alloc() is not handled in the sample of shadow variable API. It is not strictly necessary because livepatch_fix1_dummy_free() is able to handle the potential failure. But it is an example and it should use the API a clean way. Signed-off-by: Petr Mladek Reviewed-by: Joe Lawrence Acked-by: Miroslav Benes Reviewed-by: Kamalesh Babulal Signed-off-by: Jiri Kosina --- samples/livepatch/livepatch-shadow-fix1.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) (limited to 'samples') diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c index de0363b288a7..918ce17b43fd 100644 --- a/samples/livepatch/livepatch-shadow-fix1.c +++ b/samples/livepatch/livepatch-shadow-fix1.c @@ -66,6 +66,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void) { struct dummy *d; int *leak; + int **shadow_leak; d = kzalloc(sizeof(*d), GFP_KERNEL); if (!d) @@ -80,18 +81,27 @@ static struct dummy *livepatch_fix1_dummy_alloc(void) * pointer to handle resource release. */ leak = kzalloc(sizeof(*leak), GFP_KERNEL); - if (!leak) { - kfree(d); - return NULL; + if (!leak) + goto err_leak; + + shadow_leak = klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL, + shadow_leak_ctor, &leak); + if (!shadow_leak) { + pr_err("%s: failed to allocate shadow variable for the leaking pointer: dummy @ %p, leak @ %p\n", + __func__, d, leak); + goto err_shadow; } - klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL, - shadow_leak_ctor, &leak); - pr_info("%s: dummy @ %p, expires @ %lx\n", __func__, d, d->jiffies_expire); return d; + +err_shadow: + kfree(leak); +err_leak: + kfree(d); + return NULL; } static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data) -- cgit v1.2.3-59-g8ed1b