live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Mladek <pmladek@suse.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Joe Lawrence <joe.lawrence@redhat.com>, live-patching@vger.kernel.org
Subject: Re: [bug report] livepatch: Initialize shadow variables safely by a custom callback
Date: Thu, 9 Jan 2020 10:29:34 +0100	[thread overview]
Message-ID: <20200109092934.fdjgqc4es46mjpkv@pathway.suse.cz> (raw)
In-Reply-To: <20200107152337.GB27042@kadam>

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


  parent reply	other threads:[~2020-01-09  9:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2020-01-13 15:11       ` Joe Lawrence

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200109092934.fdjgqc4es46mjpkv@pathway.suse.cz \
    --to=pmladek@suse.com \
    --cc=dan.carpenter@oracle.com \
    --cc=joe.lawrence@redhat.com \
    --cc=live-patching@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).