linux-kernel - Re: [PATCH v4 1/2] Introduce klp_ops into klp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <D2C635CE-4530-492C-B9CF-CFF78FE116A1@gmail.com>
Date: Fri, 13 Sep 2024 17:46:18 +0800
From: zhang warden <zhangwarden@...il.com>
To: Miroslav Benes <mbenes@...e.cz>
Cc: Josh Poimboeuf <jpoimboe@...nel.org>,
 Jiri Kosina <jikos@...nel.org>,
 Petr Mladek <pmladek@...e.com>,
 Joe Lawrence <joe.lawrence@...hat.com>,
 live-patching@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] Introduce klp_ops into klp_func structure

Hi, Miroslav & Petr

> On Sep 6, 2024, at 17:44, zhang warden <zhangwarden@...il.com> wrote:
> 
>> 
>>>> 
>>>>> +       struct list_head func_stack;
>>>>> +       struct ftrace_ops fops;
>>>>> +};
>>>>> +
>>>>> 
>>>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>>>>> index 52426665eecc..e4572bf34316 100644
>>>>> --- a/kernel/livepatch/core.c
>>>>> +++ b/kernel/livepatch/core.c
>>>>> @@ -760,6 +760,8 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>>>>> if (!func->old_name)
>>>>> return -EINVAL;
>>>>> 
>>>>> + func->ops = NULL;
>>>>> +
>>>> 
>>>> Any reason why it is not added a couple of lines later alongside the rest 
>>>> of the initialization?
>>> 
>>> Do you mean I should add couple of lines after 'return -EINVAL' ?
>> 
>> No, I am asking if there is a reason why you added 'func->ops = NULL;' 
>> here and not right after the rest of func initializations
>> 
>>       INIT_LIST_HEAD(&func->stack_node);
>>       func->patched = false;
>>       func->transition = false;
>> 
> 

I think I found a bug in my patch.

I move struct klp_ops to klp_func. But every time, klp_func
will init klp_func->ops to NULL. Which will make the test branch
 `if(! ops)` always true in function klp_patch_func.

An alternative solution should be something like
 (as Peter suggested before) 

https://lore.kernel.org/live-patching/20240805064656.40017-1-zhangyongde.zyd@alibaba-inc.com/T/#t


 static struct klp_ops *klp_find_ops(void *old_func)
 {
       struct klp_patch *patch;
       struct klp_object *obj;
       struct klp_func *func;
       struct klp_ops *ops;
 
        klp_for_each_patch(patch)
                klp_for_each_object(patch, obj)
                        klp_for_each_func(obj, func)
                               if(func->old_func == old_func)
                                        return func->ops;
        return NULL;
 }

and  klp_ops should be initialize in klp_init_func:

        func->patched = false;
        func->transition = false;
+       func->ops = klp_find_ops(func->old_func);

Here, func->ops should not init as NULL, it should be initialize with 
the existed ops (if klp_find_ops returns NULL, this patch is the first
time to be patched).

Regards.
Wardenjohn.