[PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[Lv Yunlong]

In register_test_dev_kmod, it calls free_test_dev_kmod() to free
test_dev. But free_test_dev_kmod() can't set the original pointer
test_dev to NULL, because the test_dev was passed by it's value
not reference.

Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
---
 lib/test_kmod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 38c250fbace3..aa8a2a563d7e 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -1124,7 +1124,6 @@ static void free_test_dev_kmod(struct kmod_test_device *test_dev)
 		free_test_dev_info(test_dev);
 		kmod_config_free(test_dev);
 		vfree(test_dev);
-		test_dev = NULL;
 	}
 }
 
@@ -1149,6 +1148,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
 	if (ret) {
 		pr_err("could not register misc device: %d\n", ret);
 		free_test_dev_kmod(test_dev);
+		test_dev = NULL;
 		goto out;
 	}
 
-- 
2.25.1

Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[Luis Chamberlain]

On Thu, Mar 11, 2021 at 12:02:46AM -0800, Lv Yunlong wrote:
> In register_test_dev_kmod, it calls free_test_dev_kmod() to free
> test_dev. But free_test_dev_kmod() can't set the original pointer
> test_dev to NULL, because the test_dev was passed by it's value
> not reference.

Did you actually get a crash or something? If so can you supply the
actual log? If this is just an observation and you think this is an
issue, specifying that would help during patch review.

  Luis

> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> ---
>  lib/test_kmod.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/test_kmod.c b/lib/test_kmod.c
> index 38c250fbace3..aa8a2a563d7e 100644
> --- a/lib/test_kmod.c
> +++ b/lib/test_kmod.c
> @@ -1124,7 +1124,6 @@ static void free_test_dev_kmod(struct kmod_test_device *test_dev)
>  		free_test_dev_info(test_dev);
>  		kmod_config_free(test_dev);
>  		vfree(test_dev);
> -		test_dev = NULL;
>  	}
>  }
>  
> @@ -1149,6 +1148,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
>  	if (ret) {
>  		pr_err("could not register misc device: %d\n", ret);
>  		free_test_dev_kmod(test_dev);
> +		test_dev = NULL;
>  		goto out;
>  	}
>  
> -- 
> 2.25.1
> 
> 

Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[lyl2019]

> -----原始邮件-----
> 发件人: "Luis Chamberlain" <mcgrof@kernel.org>
> 发送时间: 2021-03-11 21:01:08 (星期四)
> 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
> 抄送: linux-kernel@vger.kernel.org
> 主题: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod
> 
> On Thu, Mar 11, 2021 at 12:02:46AM -0800, Lv Yunlong wrote:
> > In register_test_dev_kmod, it calls free_test_dev_kmod() to free
> > test_dev. But free_test_dev_kmod() can't set the original pointer
> > test_dev to NULL, because the test_dev was passed by it's value
> > not reference.
> 
> Did you actually get a crash or something? If so can you supply the
> actual log? If this is just an observation and you think this is an
> issue, specifying that would help during patch review.
> 
>   Luis
> 
> > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> > ---
> >  lib/test_kmod.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/test_kmod.c b/lib/test_kmod.c
> > index 38c250fbace3..aa8a2a563d7e 100644
> > --- a/lib/test_kmod.c
> > +++ b/lib/test_kmod.c
> > @@ -1124,7 +1124,6 @@ static void free_test_dev_kmod(struct kmod_test_device *test_dev)
> >  		free_test_dev_info(test_dev);
> >  		kmod_config_free(test_dev);
> >  		vfree(test_dev);
> > -		test_dev = NULL;
> >  	}
> >  }
> >  
> > @@ -1149,6 +1148,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
> >  	if (ret) {
> >  		pr_err("could not register misc device: %d\n", ret);
> >  		free_test_dev_kmod(test_dev);
> > +		test_dev = NULL;
> >  		goto out;
> >  	}
> >  
> > -- 
> > 2.25.1
> > 
> > 

This problem was reported by source code analyzers developed by our Security Lab(Loccs).
We have confirmed this issue before submiting the patch.

If you still have any questions about this patch, i will reply to you as soon as possible.

Thanks for your time.

Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[Luis Chamberlain]

On Thu, Mar 11, 2021 at 09:31:55PM +0800, lyl2019@mail.ustc.edu.cn wrote:
> 
> 
> 
> > -----原始邮件-----
> > 发件人: "Luis Chamberlain" <mcgrof@kernel.org>
> > 发送时间: 2021-03-11 21:01:08 (星期四)
> > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
> > 抄送: linux-kernel@vger.kernel.org
> > 主题: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod
> > 
> > On Thu, Mar 11, 2021 at 12:02:46AM -0800, Lv Yunlong wrote:
> > > In register_test_dev_kmod, it calls free_test_dev_kmod() to free
> > > test_dev. But free_test_dev_kmod() can't set the original pointer
> > > test_dev to NULL, because the test_dev was passed by it's value
> > > not reference.
> > 
> > Did you actually get a crash or something? If so can you supply the
> > actual log? If this is just an observation and you think this is an
> > issue, specifying that would help during patch review.
> > 
> >   Luis
> > 
> > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> > > ---
> > >  lib/test_kmod.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/test_kmod.c b/lib/test_kmod.c
> > > index 38c250fbace3..aa8a2a563d7e 100644
> > > --- a/lib/test_kmod.c
> > > +++ b/lib/test_kmod.c
> > > @@ -1124,7 +1124,6 @@ static void free_test_dev_kmod(struct kmod_test_device *test_dev)
> > >  		free_test_dev_info(test_dev);
> > >  		kmod_config_free(test_dev);
> > >  		vfree(test_dev);
> > > -		test_dev = NULL;
> > >  	}
> > >  }
> > >  
> > > @@ -1149,6 +1148,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
> > >  	if (ret) {
> > >  		pr_err("could not register misc device: %d\n", ret);
> > >  		free_test_dev_kmod(test_dev);
> > > +		test_dev = NULL;
> > >  		goto out;
> > >  	}
> > >  
> > > -- 
> > > 2.25.1
> > > 
> > > 
> 
> This problem was reported by source code analyzers developed by our Security Lab(Loccs).

This should be included in the commit log, please.

> We have confirmed this issue before submiting the patch.

How was this confirmed exactly.

  Luis

Re: Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[lyl2019]

> -----原始邮件-----
> 发件人: "Luis Chamberlain" <mcgrof@kernel.org>
> 发送时间: 2021-03-11 21:58:33 (星期四)
> 收件人: lyl2019@mail.ustc.edu.cn
> 抄送: linux-kernel@vger.kernel.org
> 主题: Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod
> 
> On Thu, Mar 11, 2021 at 09:31:55PM +0800, lyl2019@mail.ustc.edu.cn wrote:
> > 
> > 
> > 
> > > -----原始邮件-----
> > > 发件人: "Luis Chamberlain" <mcgrof@kernel.org>
> > > 发送时间: 2021-03-11 21:01:08 (星期四)
> > > 收件人: "Lv Yunlong" <lyl2019@mail.ustc.edu.cn>
> > > 抄送: linux-kernel@vger.kernel.org
> > > 主题: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod
> > > 
> > > On Thu, Mar 11, 2021 at 12:02:46AM -0800, Lv Yunlong wrote:
> > > > In register_test_dev_kmod, it calls free_test_dev_kmod() to free
> > > > test_dev. But free_test_dev_kmod() can't set the original pointer
> > > > test_dev to NULL, because the test_dev was passed by it's value
> > > > not reference.
> > > 
> > > Did you actually get a crash or something? If so can you supply the
> > > actual log? If this is just an observation and you think this is an
> > > issue, specifying that would help during patch review.
> > > 
> > >   Luis
> > > 
> > > > Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> > > > ---
> > > >  lib/test_kmod.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/test_kmod.c b/lib/test_kmod.c
> > > > index 38c250fbace3..aa8a2a563d7e 100644
> > > > --- a/lib/test_kmod.c
> > > > +++ b/lib/test_kmod.c
> > > > @@ -1124,7 +1124,6 @@ static void free_test_dev_kmod(struct kmod_test_device *test_dev)
> > > >  		free_test_dev_info(test_dev);
> > > >  		kmod_config_free(test_dev);
> > > >  		vfree(test_dev);
> > > > -		test_dev = NULL;
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -1149,6 +1148,7 @@ static struct kmod_test_device *register_test_dev_kmod(void)
> > > >  	if (ret) {
> > > >  		pr_err("could not register misc device: %d\n", ret);
> > > >  		free_test_dev_kmod(test_dev);
> > > > +		test_dev = NULL;
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.25.1
> > > > 
> > > > 
> > 
> > This problem was reported by source code analyzers developed by our Security Lab(Loccs).
> 
> This should be included in the commit log, please.
> 
> > We have confirmed this issue before submiting the patch.
> 
> How was this confirmed exactly.
> 
>   Luis

Thanks for your advice.

Ok, let's see the details. In the function test_kmod_init(), it calls register_test_dev_kmod()
to return a test_dev. Inside the callee function register_test_dev_kmod(), the test_dev is 
allocated by alloc_test_dev_kmod() and freed by free_test_dev_kmod(). 

Notice the implementation of free_test_dev_kmod, the 'test_dev' in this function actually is
a new variable in the parameter's stack space. Set the value of 'test_dev' inside this function 
will not affect the outside 'test_dev'.
"""
static void free_test_dev_kmod(struct kmod_test_device *test_dev)
{
	if (test_dev) {
		kfree_const(test_dev->misc_dev.name);
		test_dev->misc_dev.name = NULL;
		free_test_dev_info(test_dev);
		kmod_config_free(test_dev);
		vfree(test_dev);
		test_dev = NULL;
	}
}
"""

So, register_test_dev_kmod() will return a valid and freed test_dev, and cause use after free
in function test_kmod_init().
"""
static int __init test_kmod_init(void)
{
	test_dev = register_test_dev_kmod();
	if (!test_dev) {                     // freed and valid.
		pr_err("Cannot add first test kmod device\n");
		return -ENODEV;
	}

	ret = trigger_config_run_type(test_dev,  // cause use after free here!
					      TEST_KMOD_DRIVER, "tun");
"""

I think i should write more details in the first commit, i'm sorry for wasting your time.

Thanks.

Re: Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod

[Luis Chamberlain]

On Thu, Mar 11, 2021 at 10:40:33PM +0800, lyl2019@mail.ustc.edu.cn wrote:
> So, register_test_dev_kmod() will return a valid and freed test_dev, and cause use after free
> in function test_kmod_init().

Without looking at the details, in trying to improve the commit log
further:

Is there a way you can reproduce a real world UAF and crash? If not why not?
What is the risk of not merging this commit into the kernel tree. This
information is useful for folks to evaluate whether or not users of this
module might want to merge this and/or backport it into their testing
kernel.

If chances of this happening are 0, then this just a theoretical issue.

  Luis