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

[lyl2019]

> -----原始邮件-----
> 发件人: "Luis Chamberlain" <mcg...@kernel.org>
> 发送时间: 2021-03-11 21:58:33 (星期四)
> 收件人: lyl2...@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, lyl2...@mail.ustc.edu.cn wrote:
> > 
> > 
> > 
> > > -----原始邮件-----
> > > 发件人: "Luis Chamberlain" <mcg...@kernel.org>
> > > 发送时间: 2021-03-11 21:01:08 (星期四)
> > > 收件人: "Lv Yunlong" <lyl2...@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 <lyl2...@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.