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
In some other cases in the kernel it was okay to make this type of change but
clearly not here (see the gcc 8.1.0 patches in the kernel). Another option is
to drop strcpy and use memcpy, as Guenter has done with some other fixes. TBH I
thought that was the correct fix but made a bad assumption about the history of
the code. What do you think of the memcpy replacement?
Not good.
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.
Daniel.
_______________________________________________
Kgdb-bugreport mailing list
Kgdb-bugreport@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
