Messages in this thread | | | Date | Thu, 11 Mar 2021 22:40:33 +0800 (GMT+08:00) | From | lyl2019@mail ... | Subject | Re: Re: Re: [PATCH] lib/test_kmod: Fix a use after free in register_test_dev_kmod |
| |
> -----原始邮件----- > 发件人: "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.
| |