[PATCH] kdb: prevent possible null deref in kdb_msg_write

[PATCH] kdb: prevent possible null deref in kdb_msg_write

[Cengiz Can]

`kdb_msg_write` operates on a global `struct kgdb_io *` called
`dbg_io_ops`.

Although it is initialized in `debug_core.c`, there's a null check in
`kdb_msg_write` which implies that it can be null whenever we dereference
it in this function call.

Coverity scanner caught this as CID 1465042.

I have modified the function to bail out if `dbg_io_ops` is not properly
initialized.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 683a799618ad..85e579812458 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	if (msg_len == 0)
 		return;
 
-	if (dbg_io_ops) {
-		const char *cp = msg;
-		int len = msg_len;
+	if (!dbg_io_ops)
+		return;
 
-		while (len--) {
-			dbg_io_ops->write_char(*cp);
-			cp++;
-		}
+	const char *cp = msg;
+	int len = msg_len;
+
+	while (len--) {
+		dbg_io_ops->write_char(*cp);
+		cp++;
 	}
 
 	for_each_console(c) {
-- 
2.27.0

Re: [PATCH] kdb: prevent possible null deref in kdb_msg_write

[Daniel Thompson]

On Mon, Jun 29, 2020 at 04:59:24PM +0300, Cengiz Can wrote:
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
> 
> Although it is initialized in `debug_core.c`, there's a null check in
> `kdb_msg_write` which implies that it can be null whenever we dereference
> it in this function call.
> 
> Coverity scanner caught this as CID 1465042.
> 
> I have modified the function to bail out if `dbg_io_ops` is not properly
> initialized.

That can't possibly be the right fix!

If dbg_io_ops were NULL in this part of the code then the system
is seriously broken and we would need to panic()... but since we
know that is isn't NULL (as you said, we already checked it before
we entered kdb) then we can just remove the check.


Daniel.

> 
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..85e579812458 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	if (msg_len == 0)
>  		return;
>  
> -	if (dbg_io_ops) {
> -		const char *cp = msg;
> -		int len = msg_len;
> +	if (!dbg_io_ops)
> +		return;
>  
> -		while (len--) {
> -			dbg_io_ops->write_char(*cp);
> -			cp++;
> -		}
> +	const char *cp = msg;
> +	int len = msg_len;
> +
> +	while (len--) {
> +		dbg_io_ops->write_char(*cp);
> +		cp++;
>  	}
>  
>  	for_each_console(c) {
> -- 
> 2.27.0
> 

Re: [PATCH] kdb: prevent possible null deref in kdb_msg_write

[Petr Mladek]

On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
> 
> Although it is initialized in `debug_core.c`, there's a null check in
> `kdb_msg_write` which implies that it can be null whenever we dereference
> it in this function call.
> 
> Coverity scanner caught this as CID 1465042.
> 
> I have modified the function to bail out if `dbg_io_ops` is not properly
> initialized.
> 
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..85e579812458 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
>  	if (msg_len == 0)
>  		return;
>  
> -	if (dbg_io_ops) {
> -		const char *cp = msg;
> -		int len = msg_len;
> +	if (!dbg_io_ops)
> +		return;

This looks wrong. The message should be printed to the consoles
even when dbg_io_ops is NULL. I mean that the for_each_console(c)
cycle should always get called.

Well, the code really looks racy. dbg_io_ops is set under
kgdb_registration_lock. IMHO, it should also get accessed under this lock.

It seems that the race is possible. kdb_msg_write() is called from
vkdb_printf(). This function is serialized on more CPUs using
kdb_printf_cpu lock. But it is not serialized with
kgdb_register_io_module() and kgdb_unregister_io_module() calls.

But I might miss something. dbg_io_ops is dereferenced on many other
locations without any check.


>  
> -		while (len--) {
> -			dbg_io_ops->write_char(*cp);
> -			cp++;
> -		}
> +	const char *cp = msg;
> +	int len = msg_len;
> +
> +	while (len--) {
> +		dbg_io_ops->write_char(*cp);
> +		cp++;
>  	}
>  
>  	for_each_console(c) {

You probably got confused by this new code:

		if (c == dbg_io_ops->cons)
			continue;

It dereferences dbg_io_ops without NULL check. It should probably
get replaced by:

		if (dbg_io_ops && c == dbg_io_ops->cons)
			continue;

Daniel, Sumit, could you please put some light on this?

Best Regards,
Petr

Re: [PATCH] kdb: prevent possible null deref in kdb_msg_write

[Petr Mladek]

On Mon 2020-06-29 16:50:20, Petr Mladek wrote:
> On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> > 
> > Although it is initialized in `debug_core.c`, there's a null check in
> > `kdb_msg_write` which implies that it can be null whenever we dereference
> > it in this function call.
> > 
> > Coverity scanner caught this as CID 1465042.
> > 
> > I have modified the function to bail out if `dbg_io_ops` is not properly
> > initialized.
> > 
> > Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 683a799618ad..85e579812458 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> >  	if (msg_len == 0)
> >  		return;
> >  
> > -	if (dbg_io_ops) {
> > -		const char *cp = msg;
> > -		int len = msg_len;
> > +	if (!dbg_io_ops)
> > +		return;
> 
> This looks wrong. The message should be printed to the consoles
> even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> cycle should always get called.

Please, forget this mail. Daniel explained that dbg_io_ops must have
been set when this function gets called.

I am sorry for the noise.

Best Regards,
Petr

Re: [PATCH] kdb: prevent possible null deref in kdb_msg_write

[Daniel Thompson]

On Mon, Jun 29, 2020 at 04:50:20PM +0200, Petr Mladek wrote:
> On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> > 
> > Although it is initialized in `debug_core.c`, there's a null check in
> > `kdb_msg_write` which implies that it can be null whenever we dereference
> > it in this function call.
> > 
> > Coverity scanner caught this as CID 1465042.
> > 
> > I have modified the function to bail out if `dbg_io_ops` is not properly
> > initialized.
> > 
> > Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> > ---
> >  kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index 683a799618ad..85e579812458 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> >  	if (msg_len == 0)
> >  		return;
> >  
> > -	if (dbg_io_ops) {
> > -		const char *cp = msg;
> > -		int len = msg_len;
> > +	if (!dbg_io_ops)
> > +		return;
> 
> This looks wrong. The message should be printed to the consoles
> even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> cycle should always get called.
> 
> Well, the code really looks racy. dbg_io_ops is set under
> kgdb_registration_lock. IMHO, it should also get accessed under this lock.
> 
> It seems that the race is possible. kdb_msg_write() is called from
> vkdb_printf(). This function is serialized on more CPUs using
> kdb_printf_cpu lock. But it is not serialized with
> kgdb_register_io_module() and kgdb_unregister_io_module() calls.

We can't take the lock from the trap handler itself since we cannot
have spinlocks contended between regular threads and the debug trap
(which could be an NMI).

Instead, the call to kgdb_unregister_callbacks() at the beginning
of kgdb_unregister_io_module() should render kdb_msg_write()
unreachable prior to dbg_io_ops becoming NULL.

As it happens I am starting to believe there is a race in this area but
the race is between register/unregister calls rather than against the
trap handler (if there were register/unregister races then the trap
handler is be a potential victim of the race though).


> But I might miss something. dbg_io_ops is dereferenced on many other
> locations without any check.

There is already a paranoid "just in case there are bugs" check in
kgdb_io_ready() so in any case I think the check in kdb_msg_write() can
simply be removed.

As I said in my other post, if dbg_io_ops were ever NULL then the
system is completely hosed anyway: we can never receive the keystroke
needed to leave the debugger... and may not be able to tell anybody
why.


> >  
> > -		while (len--) {
> > -			dbg_io_ops->write_char(*cp);
> > -			cp++;
> > -		}
> > +	const char *cp = msg;
> > +	int len = msg_len;
> > +
> > +	while (len--) {
> > +		dbg_io_ops->write_char(*cp);
> > +		cp++;
> >  	}
> >  
> >  	for_each_console(c) {
> 
> You probably got confused by this new code:
> 
> 		if (c == dbg_io_ops->cons)
> 			continue;
> 
> It dereferences dbg_io_ops without NULL check. It should probably
> get replaced by:
> 
> 		if (dbg_io_ops && c == dbg_io_ops->cons)
> 			continue;
> 
> Daniel, Sumit, could you please put some light on this?

As above, I think the NULL check that confuses coverity can simply be
removed.


Daniel.

[PATCH v2] kdb: remove unnecessary null check of dbg_io_ops

[Cengiz Can]

`kdb_msg_write` operates on a global `struct kgdb_io *` called
`dbg_io_ops`.

It's initialized in `debug_core.c` and checked throughout the debug
flow.

There's a null check in `kdb_msg_write` which triggers static analyzers
and gives the (almost entirely wrong) impression that it can be null.

Coverity scanner caught this as CID 1465042.

I have removed the unnecessary null check and eliminated false-positive
forward null dereference warning.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 kernel/debug/kdb/kdb_io.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 683a799618ad..4ac59a4fbeec 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
 	if (msg_len == 0)
 		return;
 
-	if (dbg_io_ops) {
-		const char *cp = msg;
-		int len = msg_len;
+	const char *cp = msg;
+	int len = msg_len;
 
-		while (len--) {
-			dbg_io_ops->write_char(*cp);
-			cp++;
-		}
+	while (len--) {
+		dbg_io_ops->write_char(*cp);
+		cp++;
 	}
 
 	for_each_console(c) {
-- 
2.27.0

Re: [PATCH v2] kdb: remove unnecessary null check of dbg_io_ops

[Doug Anderson]

Hi,

On Mon, Jun 29, 2020 at 1:50 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  kernel/debug/kdb/kdb_io.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..4ac59a4fbeec 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
>         if (msg_len == 0)
>                 return;
>
> -       if (dbg_io_ops) {
> -               const char *cp = msg;
> -               int len = msg_len;
> +       const char *cp = msg;
> +       int len = msg_len;

kernel/debug/kdb/kdb_io.c:552:14:
warning: ISO C90 forbids mixing declarations and code
[-Wdeclaration-after-statement]

-Doug

Re: [PATCH v2] kdb: remove unnecessary null check of dbg_io_ops

[Cengiz Can]

On June 30, 2020 00:16:54 Doug Anderson <dianders@chromium.org> wrote:

> Hi,
>
> On Mon, Jun 29, 2020 at 1:50 PM Cengiz Can <cengiz@kernel.wtf> wrote:
>>
>> `kdb_msg_write` operates on a global `struct kgdb_io *` called
>> `dbg_io_ops`.
>>
>> It's initialized in `debug_core.c` and checked throughout the debug
>> flow.
>>
>> There's a null check in `kdb_msg_write` which triggers static analyzers
>> and gives the (almost entirely wrong) impression that it can be null.
>>
>> Coverity scanner caught this as CID 1465042.
>>
>> I have removed the unnecessary null check and eliminated false-positive
>> forward null dereference warning.
>>
>> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
>> ---
>> kernel/debug/kdb/kdb_io.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
>> index 683a799618ad..4ac59a4fbeec 100644
>> --- a/kernel/debug/kdb/kdb_io.c
>> +++ b/kernel/debug/kdb/kdb_io.c
>> @@ -549,14 +549,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
>> if (msg_len == 0)
>>        return;
>>
>> -       if (dbg_io_ops) {
>> -               const char *cp = msg;
>> -               int len = msg_len;
>> +       const char *cp = msg;
>> +       int len = msg_len;
>
> kernel/debug/kdb/kdb_io.c:552:14:
> warning: ISO C90 forbids mixing declarations and code
> [-Wdeclaration-after-statement]

Oops!

Sorry for that..

I admit I didn't compile check my second attempt.

/me ducks and covers

Will fix it asap.

Thanks!

>
> -Doug

Re: [PATCH] kdb: prevent possible null deref in kdb_msg_write

[Sumit Garg]

On Mon, 29 Jun 2020 at 21:07, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Mon, Jun 29, 2020 at 04:50:20PM +0200, Petr Mladek wrote:
> > On Mon 2020-06-29 16:59:24, Cengiz Can wrote:
> > > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > > `dbg_io_ops`.
> > >
> > > Although it is initialized in `debug_core.c`, there's a null check in
> > > `kdb_msg_write` which implies that it can be null whenever we dereference
> > > it in this function call.
> > >
> > > Coverity scanner caught this as CID 1465042.
> > >
> > > I have modified the function to bail out if `dbg_io_ops` is not properly
> > > initialized.
> > >
> > > Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> > > ---
> > >  kernel/debug/kdb/kdb_io.c | 15 ++++++++-------
> > >  1 file changed, 8 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > > index 683a799618ad..85e579812458 100644
> > > --- a/kernel/debug/kdb/kdb_io.c
> > > +++ b/kernel/debug/kdb/kdb_io.c
> > > @@ -549,14 +549,15 @@ static void kdb_msg_write(const char *msg, int msg_len)
> > >     if (msg_len == 0)
> > >             return;
> > >
> > > -   if (dbg_io_ops) {
> > > -           const char *cp = msg;
> > > -           int len = msg_len;
> > > +   if (!dbg_io_ops)
> > > +           return;
> >
> > This looks wrong. The message should be printed to the consoles
> > even when dbg_io_ops is NULL. I mean that the for_each_console(c)
> > cycle should always get called.
> >
> > Well, the code really looks racy. dbg_io_ops is set under
> > kgdb_registration_lock. IMHO, it should also get accessed under this lock.
> >
> > It seems that the race is possible. kdb_msg_write() is called from
> > vkdb_printf(). This function is serialized on more CPUs using
> > kdb_printf_cpu lock. But it is not serialized with
> > kgdb_register_io_module() and kgdb_unregister_io_module() calls.
>
> We can't take the lock from the trap handler itself since we cannot
> have spinlocks contended between regular threads and the debug trap
> (which could be an NMI).
>
> Instead, the call to kgdb_unregister_callbacks() at the beginning
> of kgdb_unregister_io_module() should render kdb_msg_write()
> unreachable prior to dbg_io_ops becoming NULL.
>
> As it happens I am starting to believe there is a race in this area but
> the race is between register/unregister calls rather than against the
> trap handler (if there were register/unregister races then the trap
> handler is be a potential victim of the race though).
>
>
> > But I might miss something. dbg_io_ops is dereferenced on many other
> > locations without any check.
>
> There is already a paranoid "just in case there are bugs" check in
> kgdb_io_ready() so in any case I think the check in kdb_msg_write() can
> simply be removed.
>
> As I said in my other post, if dbg_io_ops were ever NULL then the
> system is completely hosed anyway: we can never receive the keystroke
> needed to leave the debugger... and may not be able to tell anybody
> why.
>
>
> > >
> > > -           while (len--) {
> > > -                   dbg_io_ops->write_char(*cp);
> > > -                   cp++;
> > > -           }
> > > +   const char *cp = msg;
> > > +   int len = msg_len;
> > > +
> > > +   while (len--) {
> > > +           dbg_io_ops->write_char(*cp);
> > > +           cp++;
> > >     }
> > >
> > >     for_each_console(c) {
> >
> > You probably got confused by this new code:
> >
> >               if (c == dbg_io_ops->cons)
> >                       continue;
> >
> > It dereferences dbg_io_ops without NULL check. It should probably
> > get replaced by:
> >
> >               if (dbg_io_ops && c == dbg_io_ops->cons)
> >                       continue;
> >
> > Daniel, Sumit, could you please put some light on this?
>
> As above, I think the NULL check that confuses coverity can simply be
> removed.
>

+1

-Sumit

>
> Daniel.

[PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Cengiz Can]

`kdb_msg_write` operates on a global `struct kgdb_io *` called
`dbg_io_ops`.

It's initialized in `debug_core.c` and checked throughout the debug
flow.

There's a null check in `kdb_msg_write` which triggers static analyzers
and gives the (almost entirely wrong) impression that it can be null.

Coverity scanner caught this as CID 1465042.

I have removed the unnecessary null check and eliminated false-positive
forward null dereference warning.

Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
---
 kernel/debug/kdb/kdb_io.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 683a799618ad..81783ecaec58 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -545,18 +545,18 @@ static int kdb_search_string(char *searched, char *searchfor)
 static void kdb_msg_write(const char *msg, int msg_len)
 {
 	struct console *c;
+	const char *cp;
+	int len;
 
 	if (msg_len == 0)
 		return;
 
-	if (dbg_io_ops) {
-		const char *cp = msg;
-		int len = msg_len;
+	cp = msg;
+	len = msg_len;
 
-		while (len--) {
-			dbg_io_ops->write_char(*cp);
-			cp++;
-		}
+	while (len--) {
+		dbg_io_ops->write_char(*cp);
+		cp++;
 	}
 
 	for_each_console(c) {
-- 
2.27.0

Re: [PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Sumit Garg]

On Tue, 30 Jun 2020 at 14:00, Cengiz Can <cengiz@kernel.wtf> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  kernel/debug/kdb/kdb_io.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 683a799618ad..81783ecaec58 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -545,18 +545,18 @@ static int kdb_search_string(char *searched, char *searchfor)
>  static void kdb_msg_write(const char *msg, int msg_len)
>  {
>         struct console *c;
> +       const char *cp;
> +       int len;
>
>         if (msg_len == 0)
>                 return;
>
> -       if (dbg_io_ops) {
> -               const char *cp = msg;
> -               int len = msg_len;
> +       cp = msg;
> +       len = msg_len;
>
> -               while (len--) {
> -                       dbg_io_ops->write_char(*cp);
> -                       cp++;
> -               }
> +       while (len--) {
> +               dbg_io_ops->write_char(*cp);
> +               cp++;
>         }
>
>         for_each_console(c) {
> --
> 2.27.0
>

Re: [PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Andy Shevchenko]

On Tue, Jun 30, 2020 at 05:06:31PM +0530, Sumit Garg wrote:
> On Tue, 30 Jun 2020 at 14:00, Cengiz Can <cengiz@kernel.wtf> wrote:
> >
> > `kdb_msg_write` operates on a global `struct kgdb_io *` called
> > `dbg_io_ops`.
> >
> > It's initialized in `debug_core.c` and checked throughout the debug
> > flow.
> >
> > There's a null check in `kdb_msg_write` which triggers static analyzers
> > and gives the (almost entirely wrong) impression that it can be null.
> >
> > Coverity scanner caught this as CID 1465042.
> >
> > I have removed the unnecessary null check and eliminated false-positive
> > forward null dereference warning.

> > -               while (len--) {
> > -                       dbg_io_ops->write_char(*cp);
> > -                       cp++;
> > -               }
> > +       while (len--) {
> > +               dbg_io_ops->write_char(*cp);
> > +               cp++;
> >         }

Despite being in the original code this can be simple done in two lines:

	while (len--)
		dbg_io_ops->write_char(*cp++);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Doug Anderson]

Hi,

On Tue, Jun 30, 2020 at 1:30 AM Cengiz Can <cengiz@kernel.wtf> wrote:
>
> `kdb_msg_write` operates on a global `struct kgdb_io *` called
> `dbg_io_ops`.
>
> It's initialized in `debug_core.c` and checked throughout the debug
> flow.
>
> There's a null check in `kdb_msg_write` which triggers static analyzers
> and gives the (almost entirely wrong) impression that it can be null.
>
> Coverity scanner caught this as CID 1465042.
>
> I have removed the unnecessary null check and eliminated false-positive
> forward null dereference warning.
>
> Signed-off-by: Cengiz Can <cengiz@kernel.wtf>
> ---
>  kernel/debug/kdb/kdb_io.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Cengiz Can]

Hello Daniel,

On 2020-07-01 01:32, Doug Anderson wrote:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>

Wanted to ask about the status of this proposed patch.

I have checked your tree in git.kernel.org but you might be
collecting them somewhere else perhaps.

Thank you for your time

-- 
Cengiz Can
@cengiz_io

Re: [PATCH v3] kdb: remove unnecessary null check of dbg_io_ops

[Daniel Thompson]

On Fri, Jul 10, 2020 at 03:15:55PM +0300, Cengiz Can wrote:
> Hello Daniel,
> 
> On 2020-07-01 01:32, Doug Anderson wrote:
> > 
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> 
> Wanted to ask about the status of this proposed patch.
> 
> I have checked your tree in git.kernel.org but you might be
> collecting them somewhere else perhaps.

It's applied... but then holiday happened. Should be pushed out soon.


Daniel.