linux-kernel - Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc

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] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 7 Jan 2023 15:08:51 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Quanfa Fu <quanfafu@...il.com>
Cc:     rostedt@...dmis.org, mhiramat@...nel.org,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] tracing/eprobe: Replace kzalloc with kmalloc

Le 07/01/2023 à 13:23, Quanfa Fu a écrit :
> On Sat, Jan 7, 2023 at 4:42 PM Christophe JAILLET
> <christophe.jaillet@...adoo.fr> wrote:
>>
>> Le 07/01/2023 à 04:45, Quanfa Fu a écrit :
>>> Since this memory will be filled soon below, I feel that there is
>>> no need for a memory of all zeros here. 'snprintf' does not return
>>> negative num according to ISO C99, so I feel that no judgment is
>>> needed here.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Quanfa Fu <quanfafu@...il.com>
>>> ---
>>>    kernel/trace/trace_eprobe.c | 4 +---
>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
>>> index 352b65e2b910..cd1d271a74e7 100644
>>> --- a/kernel/trace/trace_eprobe.c
>>> +++ b/kernel/trace/trace_eprobe.c
>>> @@ -917,15 +917,13 @@ static int trace_eprobe_parse_filter(struct trace_eprobe *ep, int argc, const ch
>>>        for (i = 0; i < argc; i++)
>>>                len += strlen(argv[i]) + 1;
>>>
>>> -     ep->filter_str = kzalloc(len, GFP_KERNEL);
>>> +     ep->filter_str = kmalloc(len, GFP_KERNEL);
>>>        if (!ep->filter_str)
>>>                return -ENOMEM;
>>>
>>>        p = ep->filter_str;
>>>        for (i = 0; i < argc; i++) {
>>>                ret = snprintf(p, len, "%s ", argv[i]);
>>> -             if (ret < 0)
>>> -                     goto error;
>>>                if (ret > len) {
>>
> 
> I think that:
> 
> for example, argc = 2, argv = {"a", "b"};
> 
> Before the loop
> ===============
> len = (1 + 1) + (1 + 1) = 4;
> ep->filter_str = 0x00 0x00 0x00 0x00
>                         ^
>                          |__ p
> 
> After the loop:
> ===============
> i = 1, snprintf write: 'a' and ' ', so ret = 2
> i = 2, snprintf write: 'b' and ' ', so ret = 2
> ===============

Ok, I missed the space after the %s.

Sorry for the noise.

>   Since the length of the last argv is not enough
> to write, the space is replaced by null
> 
> ep->filter_str = 0x61 0x20 0x62 0x00
>                                                            ^
>                                                            |__ p
> p = ep->fiter_str + 2 (ret1) +2 (ret2) = ep->filter_str + 4
> ===============
> so After p[-1] = *(ep->filter_str + 3) = '\0';
> ep->filter_str = 0x61 0x20 0x62 0x00
> 
> According to ISO C99: " If the output was truncated due to this limit
> then the return value is the number of characters (excluding the
> terminating null byte) which would have been written to the final
> string if enough  space  had been available"
> 
> The last snprintf will end with 'NULL', so I think p[-1] = '\0' can
> also be deleted

Hmm, now that I see it, I think that it is there to remove the last 
space (even if there shouldn't be any because the last snprintf will be 
truncated).

Code LGTM as-is, even if puzzling.

CJ

>