On 09/10/2018 12:10 PM, Daniel Thompson wrote:
> On 10/09/18 11:42, Prarit Bhargava wrote:
>>
>>
>> On 09/10/2018 04:30 AM, Daniel Thompson wrote:
>>> On Fri, Sep 07, 2018 at 06:08:56AM -0400, Prarit Bhargava wrote:
>>>> gcc 8.1.0 warns with:
>>>>
>>>> kernel/debug/kdb/kdb_support.c: In function ‘kallsyms_symbol_next’:
>>>> kernel/debug/kdb/kdb_support.c:239:4: warning: ‘strncpy’ specified bound
>>>> depends on the length of the source argument [-Wstringop-overflow=]
>>>> strncpy(prefix_name, name, strlen(name)+1);
>>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> kernel/debug/kdb/kdb_support.c:239:31: note: length computed here
>>>>
>>>> The strings do not need to be zero padded so use strlcpy() instead.
>>>>
>>>> Signed-off-by: Prarit Bhargava <pra...@redhat.com>
>>>> Cc: Jonathan Toppins <jtopp...@redhat.com>
>>>> Cc: Jason Wessel <jason.wes...@windriver.com>
>>>> Cc: Daniel Thompson <daniel.thomp...@linaro.org>
>>>> ---
>>>> kernel/debug/kdb/kdb_support.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/debug/kdb/kdb_support.c
>>>> b/kernel/debug/kdb/kdb_support.c
>>>> index 990b3cc526c8..1ad4370ccbf0 100644
>>>> --- a/kernel/debug/kdb/kdb_support.c
>>>> +++ b/kernel/debug/kdb/kdb_support.c
>>>> @@ -236,7 +236,7 @@ int kallsyms_symbol_next(char *prefix_name, int flag)
>>>> while ((name = kdb_walk_kallsyms(&pos))) {
>>>> if (strncmp(name, prefix_name, prefix_len) == 0) {
>>>> - strncpy(prefix_name, name, strlen(name)+1);
>>>> + strlcpy(prefix_name, name, strlen(name)+1);
>>>
>>> How does this *fix* the warning?
>>>
>>> The warning occurs because a "safe" string copy function is incorrectly
>>> using the length of the second argument as the length (i.e. it is simply
>>> an inefficient implementation of strcpy). The code is still bogus
>>> whether you use strncpy, strlcpy or strscpy. All we are doing here is
>>> kicking the ball down the road until someone teaches gcc 9+ about
>>> strlcpy()!
>>>
>>
>> Hi Daniel, you are correct in that, however name is always smaller in size
>> than
>> prefix_name[1]. I assumed (badly) that the code had taken the sizes into
>> account when written but wasn't sure, hence the simple strlcpy() patch.
>
> I'm not sure that is the case. kallsyms_symbol_complete() works with the same
> buffers and has proper overflow checking.
>
> I think if the user tab completes at the end of a long line there can be an
> overflow.
>
> There's a previous thread about this:
> https://lkml.org/lkml/2018/5/29/38
>
Thanks for the link. I was surprised that no one else had commented on the
warning & had attempted a fix.
<snip>
> As I said before:
>> My understanding is that the only way to make this overflow safe is to
>> change the signature of kallsyms_symbol_next() so it takes a max_len
>> argument similar to what is done for kallsyms_symbol_complete().
>>
>> It might even be worth using strscpy() here and propagating the -E2BIG
>> to the caller. That allows the caller to print the partial symbol and
>> an elipsis to show the user that the symbol has been truncated.
>
> Back in May Nick and I didn't explicitly agree who would tackle the bug and it
> got dropped. So... do you want to take a crack at this or shall I do it.
>
Thanks for the info. I'll take a look at it today.
P.
>
> Daniel.
>
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
