* [bug report] livepatch: Initialize shadow variables safely by a custom callback
@ 2020-01-07 13:29 Dan Carpenter
2020-01-07 15:06 ` Joe Lawrence
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2020-01-07 13:29 UTC (permalink / raw)
To: pmladek; +Cc: live-patching
Hello Petr Mladek,
The patch e91c2518a5d2: "livepatch: Initialize shadow variables
safely by a custom callback" from Apr 16, 2018, 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)
samples/livepatch/livepatch-shadow-fix1.c
53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
54 {
55 void **shadow_leak = shadow_data;
56 void *leak = ctor_data;
57
58 *shadow_leak = leak;
59 return 0;
60 }
61
62 static struct dummy *livepatch_fix1_dummy_alloc(void)
63 {
64 struct dummy *d;
65 void *leak;
66
67 d = kzalloc(sizeof(*d), GFP_KERNEL);
68 if (!d)
69 return NULL;
70
71 d->jiffies_expire = jiffies +
72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
73
74 /*
75 * Patch: save the extra memory location into a SV_LEAK shadow
76 * variable. A patched dummy_free routine can later fetch this
77 * pointer to handle resource release.
78 */
79 leak = kzalloc(sizeof(int), GFP_KERNEL);
80 if (!leak) {
81 kfree(d);
82 return NULL;
83 }
84
85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
^^^^^^^^^^^^
This doesn't seem right at all? Leak is a pointer. Why is leak a void
pointer instead of an int pointer?
86 shadow_leak_ctor, leak);
87
88 pr_info("%s: dummy @ %p, expires @ %lx\n",
89 __func__, d, d->jiffies_expire);
90
91 return d;
92 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
2020-01-07 13:29 [bug report] livepatch: Initialize shadow variables safely by a custom callback Dan Carpenter
@ 2020-01-07 15:06 ` Joe Lawrence
2020-01-07 15:23 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Joe Lawrence @ 2020-01-07 15:06 UTC (permalink / raw)
To: Dan Carpenter, pmladek; +Cc: live-patching
On 1/7/20 8:29 AM, Dan Carpenter wrote:
> Hello Petr Mladek,
>
> The patch e91c2518a5d2: "livepatch: Initialize shadow variables
> safely by a custom callback" from Apr 16, 2018, 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)
>
> samples/livepatch/livepatch-shadow-fix1.c
> 53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> 54 {
> 55 void **shadow_leak = shadow_data;
> 56 void *leak = ctor_data;
> 57
> 58 *shadow_leak = leak;
> 59 return 0;
> 60 }
> 61
> 62 static struct dummy *livepatch_fix1_dummy_alloc(void)
> 63 {
> 64 struct dummy *d;
> 65 void *leak;
> 66
> 67 d = kzalloc(sizeof(*d), GFP_KERNEL);
> 68 if (!d)
> 69 return NULL;
> 70
> 71 d->jiffies_expire = jiffies +
> 72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
> 73
> 74 /*
> 75 * Patch: save the extra memory location into a SV_LEAK shadow
> 76 * variable. A patched dummy_free routine can later fetch this
> 77 * pointer to handle resource release.
> 78 */
> 79 leak = kzalloc(sizeof(int), GFP_KERNEL);
> 80 if (!leak) {
> 81 kfree(d);
> 82 return NULL;
> 83 }
> 84
> 85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
> ^^^^^^^^^^^^
> This doesn't seem right at all? Leak is a pointer. Why is leak a void
> pointer instead of an int pointer?
>
Hi Dan,
If I remember this code correctly, the shadow variable is tracking the
pointer value itself and not its contents, so sizeof(leak) should be
correct for the shadow variable data size.
(For kernel/livepatch/shadow.c :: __klp_shadow_get_or_alloc() creates
new struct klp_shadow with .data[size] to accommodate its meta-data plus
the desired data).
Why isn't leak an int pointer? I don't remember why, according to git
history it's been that way since the beginning. I think it was coded to
say, "Give me some storage, any size an int will do. I'm not going to
touch it, but I want to demonstrate a memory leak".
Would modifying the pointer type satisfy the static code complaint?
Since the warning is about a size mismatch, what are the parameters that
it is keying on? Does it expect to see the typical allocation pattern like:
int *foo = alloc(sizeof(*foo))
and not:
int *foo = alloc(sizeof(foo))
Thanks,
-- Joe
> 86 shadow_leak_ctor, leak);
> 87
> 88 pr_info("%s: dummy @ %p, expires @ %lx\n",
> 89 __func__, d, d->jiffies_expire);
> 90
> 91 return d;
> 92 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
2020-01-07 15:06 ` Joe Lawrence
@ 2020-01-07 15:23 ` Dan Carpenter
2020-01-07 15:43 ` Joe Lawrence
2020-01-09 9:29 ` Petr Mladek
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-01-07 15:23 UTC (permalink / raw)
To: Joe Lawrence; +Cc: pmladek, live-patching
On Tue, Jan 07, 2020 at 10:06:21AM -0500, Joe Lawrence wrote:
> On 1/7/20 8:29 AM, Dan Carpenter wrote:
> > Hello Petr Mladek,
> >
> > The patch e91c2518a5d2: "livepatch: Initialize shadow variables
> > safely by a custom callback" from Apr 16, 2018, 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)
> >
> > samples/livepatch/livepatch-shadow-fix1.c
> > 53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> > 54 {
> > 55 void **shadow_leak = shadow_data;
> > 56 void *leak = ctor_data;
> > 57
> > 58 *shadow_leak = leak;
> > 59 return 0;
> > 60 }
> > 61
> > 62 static struct dummy *livepatch_fix1_dummy_alloc(void)
> > 63 {
> > 64 struct dummy *d;
> > 65 void *leak;
> > 66
> > 67 d = kzalloc(sizeof(*d), GFP_KERNEL);
> > 68 if (!d)
> > 69 return NULL;
> > 70
> > 71 d->jiffies_expire = jiffies +
> > 72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
> > 73
> > 74 /*
> > 75 * Patch: save the extra memory location into a SV_LEAK shadow
> > 76 * variable. A patched dummy_free routine can later fetch this
> > 77 * pointer to handle resource release.
> > 78 */
> > 79 leak = kzalloc(sizeof(int), GFP_KERNEL);
> > 80 if (!leak) {
> > 81 kfree(d);
> > 82 return NULL;
> > 83 }
> > 84
> > 85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
> > ^^^^^^^^^^^^
> > This doesn't seem right at all? Leak is a pointer. Why is leak a void
> > pointer instead of an int pointer?
> >
>
> Hi Dan,
>
> If I remember this code correctly, the shadow variable is tracking the
> pointer value itself and not its contents, so sizeof(leak) should be correct
> for the shadow variable data size.
>
> (For kernel/livepatch/shadow.c :: __klp_shadow_get_or_alloc() creates new
> struct klp_shadow with .data[size] to accommodate its meta-data plus the
> desired data).
>
> Why isn't leak an int pointer? I don't remember why, according to git
> history it's been that way since the beginning. I think it was coded to
> say, "Give me some storage, any size an int will do. I'm not going to touch
> it, but I want to demonstrate a memory leak".
>
> Would modifying the pointer type satisfy the static code complaint?
>
> Since the warning is about a size mismatch, what are the parameters that it
> is keying on? Does it expect to see the typical allocation pattern like:
>
> int *foo = alloc(sizeof(*foo))
>
> and not:
>
> int *foo = alloc(sizeof(foo))
>
It looks at places which call klp_shadow_alloc() and says that sometimes
the third argument is the size of the last argument. Then it complains
when a caller doesn't match.
I could just make klp_shadow_alloc() an exception.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
2020-01-07 15:23 ` Dan Carpenter
@ 2020-01-07 15:43 ` Joe Lawrence
2020-01-09 9:29 ` Petr Mladek
1 sibling, 0 replies; 6+ messages in thread
From: Joe Lawrence @ 2020-01-07 15:43 UTC (permalink / raw)
To: Dan Carpenter; +Cc: pmladek, live-patching
On 1/7/20 10:23 AM, Dan Carpenter wrote:
> On Tue, Jan 07, 2020 at 10:06:21AM -0500, Joe Lawrence wrote:
>> On 1/7/20 8:29 AM, Dan Carpenter wrote:
>>> Hello Petr Mladek,
>>>
>>> The patch e91c2518a5d2: "livepatch: Initialize shadow variables
>>> safely by a custom callback" from Apr 16, 2018, 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)
>>>
>>> samples/livepatch/livepatch-shadow-fix1.c
>>> 53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
>>> 54 {
>>> 55 void **shadow_leak = shadow_data;
>>> 56 void *leak = ctor_data;
>>> 57
>>> 58 *shadow_leak = leak;
>>> 59 return 0;
>>> 60 }
>>> 61
>>> 62 static struct dummy *livepatch_fix1_dummy_alloc(void)
>>> 63 {
>>> 64 struct dummy *d;
>>> 65 void *leak;
>>> 66
>>> 67 d = kzalloc(sizeof(*d), GFP_KERNEL);
>>> 68 if (!d)
>>> 69 return NULL;
>>> 70
>>> 71 d->jiffies_expire = jiffies +
>>> 72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
>>> 73
>>> 74 /*
>>> 75 * Patch: save the extra memory location into a SV_LEAK shadow
>>> 76 * variable. A patched dummy_free routine can later fetch this
>>> 77 * pointer to handle resource release.
>>> 78 */
>>> 79 leak = kzalloc(sizeof(int), GFP_KERNEL);
>>> 80 if (!leak) {
>>> 81 kfree(d);
>>> 82 return NULL;
>>> 83 }
>>> 84
>>> 85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
>>> ^^^^^^^^^^^^
>>> This doesn't seem right at all? Leak is a pointer. Why is leak a void
>>> pointer instead of an int pointer?
>>>
>>
>> Hi Dan,
>>
>> If I remember this code correctly, the shadow variable is tracking the
>> pointer value itself and not its contents, so sizeof(leak) should be correct
>> for the shadow variable data size.
>>
>> (For kernel/livepatch/shadow.c :: __klp_shadow_get_or_alloc() creates new
>> struct klp_shadow with .data[size] to accommodate its meta-data plus the
>> desired data).
>>
>> Why isn't leak an int pointer? I don't remember why, according to git
>> history it's been that way since the beginning. I think it was coded to
>> say, "Give me some storage, any size an int will do. I'm not going to touch
>> it, but I want to demonstrate a memory leak".
>>
>> Would modifying the pointer type satisfy the static code complaint?
>>
>> Since the warning is about a size mismatch, what are the parameters that it
>> is keying on? [ ... snip ... ]
>
> It looks at places which call klp_shadow_alloc() and says that sometimes
> the third argument is the size of the last argument. Then it complains
> when a caller doesn't match.
>
> I could just make klp_shadow_alloc() an exception.
>
Ah, I see. It must also be checking that the last two arguments (ctor,
ctor_data) are non-null, as that's the simplified calling sequence.
/me runs cscope to find an example ...
Well that's interesting, there really aren't any other
klp_shadow_alloc() callers aside from
lib/livepatch/test_klp_shadow_vars.c, which hides it in a wrapper
routine that probably dodges the static code check.
For a typical out-of-tree example [1], we do a lot of this:
newpid = klp_shadow_get_or_alloc(p, 0, sizeof(*newpid),
GFP_KERNEL, NULL, NULL);
if (newpid)
*newpid = ctr++;
as we don't always need the constructor / destructor callbacks for
simple shadow variables.
Since the ctor/dtor callback part of the API was provided by Petr, I'll
let him chime in what the code checker should be warning about here. I
think it may be more complicated than it's worth, but maybe he has
another idea.
Regards,
-- Joe
[1]
https://github.com/dynup/kpatch/blob/master/test/integration/centos-7/shadow-newpid.patch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
2020-01-07 15:23 ` Dan Carpenter
2020-01-07 15:43 ` Joe Lawrence
@ 2020-01-09 9:29 ` Petr Mladek
2020-01-13 15:11 ` Joe Lawrence
1 sibling, 1 reply; 6+ messages in thread
From: Petr Mladek @ 2020-01-09 9:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Joe Lawrence, live-patching
On Tue 2020-01-07 18:23:37, Dan Carpenter wrote:
> On Tue, Jan 07, 2020 at 10:06:21AM -0500, Joe Lawrence wrote:
> > On 1/7/20 8:29 AM, Dan Carpenter wrote:
> > > Hello Petr Mladek,
> > >
> > > The patch e91c2518a5d2: "livepatch: Initialize shadow variables
> > > safely by a custom callback" from Apr 16, 2018, 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)
> > >
> > > samples/livepatch/livepatch-shadow-fix1.c
> > > 53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> > > 54 {
> > > 55 void **shadow_leak = shadow_data;
> > > 56 void *leak = ctor_data;
> > > 57
> > > 58 *shadow_leak = leak;
> > > 59 return 0;
> > > 60 }
> > > 61
> > > 62 static struct dummy *livepatch_fix1_dummy_alloc(void)
> > > 63 {
> > > 64 struct dummy *d;
> > > 65 void *leak;
> > > 66
> > > 67 d = kzalloc(sizeof(*d), GFP_KERNEL);
> > > 68 if (!d)
> > > 69 return NULL;
> > > 70
> > > 71 d->jiffies_expire = jiffies +
> > > 72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
> > > 73
> > > 74 /*
> > > 75 * Patch: save the extra memory location into a SV_LEAK shadow
> > > 76 * variable. A patched dummy_free routine can later fetch this
> > > 77 * pointer to handle resource release.
> > > 78 */
> > > 79 leak = kzalloc(sizeof(int), GFP_KERNEL);
> > > 80 if (!leak) {
> > > 81 kfree(d);
> > > 82 return NULL;
> > > 83 }
> > > 84
> > > 85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
> > > ^^^^^^^^^^^^
> > > This doesn't seem right at all? Leak is a pointer. Why is leak a void
> > > pointer instead of an int pointer?
> > >
> >
> > Hi Dan,
> >
> > If I remember this code correctly, the shadow variable is tracking the
> > pointer value itself and not its contents, so sizeof(leak) should be correct
> > for the shadow variable data size.
> >
> > (For kernel/livepatch/shadow.c :: __klp_shadow_get_or_alloc() creates new
> > struct klp_shadow with .data[size] to accommodate its meta-data plus the
> > desired data).
> >
> > Why isn't leak an int pointer? I don't remember why, according to git
> > history it's been that way since the beginning. I think it was coded to
> > say, "Give me some storage, any size an int will do. I'm not going to touch
> > it, but I want to demonstrate a memory leak".
> >
> > Would modifying the pointer type satisfy the static code complaint?
> >
> > Since the warning is about a size mismatch, what are the parameters that it
> > is keying on? Does it expect to see the typical allocation pattern like:
> >
> > int *foo = alloc(sizeof(*foo))
> >
> > and not:
> >
> > int *foo = alloc(sizeof(foo))
> >
>
> It looks at places which call klp_shadow_alloc() and says that sometimes
> the third argument is the size of the last argument. Then it complains
> when a caller doesn't match.
I think that this is the problem. 3rd argument is size of the
data. The last argument should be pointer to the data.
In our case, the data is pointer to the integer. We correctly
pass the size of the pointer but we pass the pointer directly.
It works because shadow_leak_ctor() is aware of this. But
it is semantically wrong.
I propose the following patch. It should probably get split
into 2 or 3 patches. In addition, we should fix
lib/livepatch/test_klp_shadow_vars.c and use the API
a clean way there as well.
I could prepare a proper patchset if you agree with
the idea. And if it actually fixes the reported error
message.
Here is a RFC patch:
From ab6cd83f6a46894c764adf9315db99ce52a9283b Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 8 Jan 2020 15:44:33 +0100
Subject: [RFC 1/1] livepatch/samples: Correctly use leak variable as a
pointer to int
The commit e91c2518a5d2 ("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 if 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
insted of pointer to the data. But it is semantically wrong and
confusing.
The minimal fix is to pass poiter to the poitner. Even better is
using the correct type: int pointer instead of void poiter.
In addition there should be some check of potential failures.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
samples/livepatch/livepatch-shadow-fix1.c | 38 +++++++++++++++++++++----------
samples/livepatch/livepatch-shadow-fix2.c | 4 ++--
samples/livepatch/livepatch-shadow-mod.c | 2 +-
3 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index e89ca4546114..a02371cf34d3 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -52,17 +52,21 @@ 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;
+ if (!ctor_data)
+ return -EINVAL;
+
+ *shadow_leak = *leak;
return 0;
}
static struct dummy *livepatch_fix1_dummy_alloc(void)
{
struct dummy *d;
- void *leak;
+ int *leak;
+ int **shadow_leak;
d = kzalloc(sizeof(*d), GFP_KERNEL);
if (!d)
@@ -77,24 +81,34 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
* pointer to handle resource release.
*/
leak = kzalloc(sizeof(int), GFP_KERNEL);
- if (!leak) {
- kfree(d);
- return NULL;
- }
+ if (!leak)
+ goto err_leak;
- klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
- shadow_leak_ctor, 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;
+ }
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)
{
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 +117,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..19c3a6824b64 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)
--
2.16.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
2020-01-09 9:29 ` Petr Mladek
@ 2020-01-13 15:11 ` Joe Lawrence
0 siblings, 0 replies; 6+ messages in thread
From: Joe Lawrence @ 2020-01-13 15:11 UTC (permalink / raw)
To: Petr Mladek; +Cc: Dan Carpenter, live-patching
On Thu, Jan 09, 2020 at 10:29:34AM +0100, Petr Mladek wrote:
> On Tue 2020-01-07 18:23:37, Dan Carpenter wrote:
> > On Tue, Jan 07, 2020 at 10:06:21AM -0500, Joe Lawrence wrote:
> > > On 1/7/20 8:29 AM, Dan Carpenter wrote:
> > > > Hello Petr Mladek,
> > > >
> > > > The patch e91c2518a5d2: "livepatch: Initialize shadow variables
> > > > safely by a custom callback" from Apr 16, 2018, 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)
> > > >
> > > > samples/livepatch/livepatch-shadow-fix1.c
> > > > 53 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> > > > 54 {
> > > > 55 void **shadow_leak = shadow_data;
> > > > 56 void *leak = ctor_data;
> > > > 57
> > > > 58 *shadow_leak = leak;
> > > > 59 return 0;
> > > > 60 }
> > > > 61
> > > > 62 static struct dummy *livepatch_fix1_dummy_alloc(void)
> > > > 63 {
> > > > 64 struct dummy *d;
> > > > 65 void *leak;
> > > > 66
> > > > 67 d = kzalloc(sizeof(*d), GFP_KERNEL);
> > > > 68 if (!d)
> > > > 69 return NULL;
> > > > 70
> > > > 71 d->jiffies_expire = jiffies +
> > > > 72 msecs_to_jiffies(1000 * EXPIRE_PERIOD);
> > > > 73
> > > > 74 /*
> > > > 75 * Patch: save the extra memory location into a SV_LEAK shadow
> > > > 76 * variable. A patched dummy_free routine can later fetch this
> > > > 77 * pointer to handle resource release.
> > > > 78 */
> > > > 79 leak = kzalloc(sizeof(int), GFP_KERNEL);
> > > > 80 if (!leak) {
> > > > 81 kfree(d);
> > > > 82 return NULL;
> > > > 83 }
> > > > 84
> > > > 85 klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
> > > > ^^^^^^^^^^^^
> > > > This doesn't seem right at all? Leak is a pointer. Why is leak a void
> > > > pointer instead of an int pointer?
> > > >
> > >
> > > Hi Dan,
> > >
> > > If I remember this code correctly, the shadow variable is tracking the
> > > pointer value itself and not its contents, so sizeof(leak) should be correct
> > > for the shadow variable data size.
> > >
> > > (For kernel/livepatch/shadow.c :: __klp_shadow_get_or_alloc() creates new
> > > struct klp_shadow with .data[size] to accommodate its meta-data plus the
> > > desired data).
> > >
> > > Why isn't leak an int pointer? I don't remember why, according to git
> > > history it's been that way since the beginning. I think it was coded to
> > > say, "Give me some storage, any size an int will do. I'm not going to touch
> > > it, but I want to demonstrate a memory leak".
> > >
> > > Would modifying the pointer type satisfy the static code complaint?
> > >
> > > Since the warning is about a size mismatch, what are the parameters that it
> > > is keying on? Does it expect to see the typical allocation pattern like:
> > >
> > > int *foo = alloc(sizeof(*foo))
> > >
> > > and not:
> > >
> > > int *foo = alloc(sizeof(foo))
> > >
> >
> > It looks at places which call klp_shadow_alloc() and says that sometimes
> > the third argument is the size of the last argument. Then it complains
> > when a caller doesn't match.
>
> I think that this is the problem. 3rd argument is size of the
> data. The last argument should be pointer to the data.
>
> In our case, the data is pointer to the integer. We correctly
> pass the size of the pointer but we pass the pointer directly.
> It works because shadow_leak_ctor() is aware of this. But
> it is semantically wrong.
>
> I propose the following patch. It should probably get split
> into 2 or 3 patches. In addition, we should fix
> lib/livepatch/test_klp_shadow_vars.c and use the API
> a clean way there as well.
>
> I could prepare a proper patchset if you agree with
> the idea. And if it actually fixes the reported error
> message.
>
> Here is a RFC patch:
>
> From ab6cd83f6a46894c764adf9315db99ce52a9283b Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@suse.com>
> Date: Wed, 8 Jan 2020 15:44:33 +0100
> Subject: [RFC 1/1] livepatch/samples: Correctly use leak variable as a
> pointer to int
>
> The commit e91c2518a5d2 ("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 if pointer). But
^^
nit: s/if/of
> 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
> insted of pointer to the data. But it is semantically wrong and
nit: s/insted/instead
> confusing.
>
> The minimal fix is to pass poiter to the poitner. Even better is
nit: s/poiter/pointer ^^^^^^ ^^^^^^^
> using the correct type: int pointer instead of void poiter.
nit: same ^^^^^^
>
> In addition there should be some check of potential failures.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> samples/livepatch/livepatch-shadow-fix1.c | 38 +++++++++++++++++++++----------
> samples/livepatch/livepatch-shadow-fix2.c | 4 ++--
> samples/livepatch/livepatch-shadow-mod.c | 2 +-
> 3 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> index e89ca4546114..a02371cf34d3 100644
> --- a/samples/livepatch/livepatch-shadow-fix1.c
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> @@ -52,17 +52,21 @@ 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;
> + if (!ctor_data)
> + return -EINVAL;
> +
> + *shadow_leak = *leak;
> return 0;
> }
>
> static struct dummy *livepatch_fix1_dummy_alloc(void)
> {
> struct dummy *d;
> - void *leak;
> + int *leak;
> + int **shadow_leak;
>
> d = kzalloc(sizeof(*d), GFP_KERNEL);
> if (!d)
> @@ -77,24 +81,34 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
> * pointer to handle resource release.
> */
> leak = kzalloc(sizeof(int), GFP_KERNEL);
> - if (!leak) {
> - kfree(d);
> - return NULL;
> - }
> + if (!leak)
> + goto err_leak;
>
> - klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
> - shadow_leak_ctor, 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",
Perhaps in a future clean up, should we consider using %px for printing
these debug pointer values?
> + __func__, d, leak);
> + goto err_shadow;
> + }
>
> 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)
> {
> 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 +117,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..19c3a6824b64 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)
> --
> 2.16.4
>
Hi Petr, this clean-up looks good to me, thanks for taking care of it
and adding the additional shadow variable allocation check. With a few
minor spelling fixes I think it would be good to go.
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
-- Joe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-13 15:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-07 13:29 [bug report] livepatch: Initialize shadow variables safely by a custom callback Dan Carpenter
2020-01-07 15:06 ` Joe Lawrence
2020-01-07 15:23 ` Dan Carpenter
2020-01-07 15:43 ` Joe Lawrence
2020-01-09 9:29 ` Petr Mladek
2020-01-13 15:11 ` Joe Lawrence
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).