[PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 29 Nov 2017 17:30:36 +0100

Move two debug messages so that a null pointer access can not happen
for the variable "ch" in these functions.

This issue was detected by using the Coccinelle software.

Fixes: 669fef464468d3f02d60a5cf725fc097e03c5cb8 ("serial: jsm: Convert jsm_printk to jsm_dbg")

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/tty/serial/jsm/jsm_tty.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
index 469927d37b41..a34eed7344e5 100644
--- a/drivers/tty/serial/jsm/jsm_tty.c
+++ b/drivers/tty/serial/jsm/jsm_tty.c
@@ -521,11 +521,10 @@ void jsm_input(struct jsm_channel *ch)
 	int s = 0;
 	int i = 0;
 
-	jsm_dbg(READ, &ch->ch_bd->pci_dev, "start\n");
-
 	if (!ch)
 		return;
 
+	jsm_dbg(READ, &ch->ch_bd->pci_dev, "start\n");
 	port = &ch->uart_port.state->port;
 	tp = port->tty;
 
@@ -647,10 +646,10 @@ static void jsm_carrier(struct jsm_channel *ch)
 	int virt_carrier = 0;
 	int phys_carrier = 0;
 
-	jsm_dbg(CARR, &ch->ch_bd->pci_dev, "start\n");
 	if (!ch)
 		return;
 
+	jsm_dbg(CARR, &ch->ch_bd->pci_dev, "start\n");
 	bd = ch->ch_bd;
 
 	if (!bd)
-- 
2.15.0

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Joe Perches]

On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 29 Nov 2017 17:30:36 +0100
> 
> Move two debug messages so that a null pointer access can not happen
> for the variable "ch" in these functions.

An actual defect fix!

Here you could probably cc stable too.

> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 669fef464468d3f02d60a5cf725fc097e03c5cb8 ("serial: jsm: Convert jsm_printk to jsm_dbg")
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/tty/serial/jsm/jsm_tty.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
> index 469927d37b41..a34eed7344e5 100644
> --- a/drivers/tty/serial/jsm/jsm_tty.c
> +++ b/drivers/tty/serial/jsm/jsm_tty.c
> @@ -521,11 +521,10 @@ void jsm_input(struct jsm_channel *ch)
>  	int s = 0;
>  	int i = 0;
>  
> -	jsm_dbg(READ, &ch->ch_bd->pci_dev, "start\n");
> -
>  	if (!ch)
>  		return;
>  
> +	jsm_dbg(READ, &ch->ch_bd->pci_dev, "start\n");
>  	port = &ch->uart_port.state->port;
>  	tp = port->tty;
>  
> @@ -647,10 +646,10 @@ static void jsm_carrier(struct jsm_channel *ch)
>  	int virt_carrier = 0;
>  	int phys_carrier = 0;
>  
> -	jsm_dbg(CARR, &ch->ch_bd->pci_dev, "start\n");
>  	if (!ch)
>  		return;
>  
> +	jsm_dbg(CARR, &ch->ch_bd->pci_dev, "start\n");
>  	bd = ch->ch_bd;
>  
>  	if (!bd)

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Greg Kroah-Hartman]

On Wed, Nov 29, 2017 at 09:23:07AM -0800, Joe Perches wrote:
> On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 29 Nov 2017 17:30:36 +0100
> > 
> > Move two debug messages so that a null pointer access can not happen
> > for the variable "ch" in these functions.
> 
> An actual defect fix!

Nope, not at all, this does not "fix" anything.

> Here you could probably cc stable too.

Nope, not worth it.

greg k-h

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Joe Perches]

On Wed, 2017-11-29 at 17:35 +0000, Greg Kroah-Hartman wrote:
> On Wed, Nov 29, 2017 at 09:23:07AM -0800, Joe Perches wrote:
> > On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > Date: Wed, 29 Nov 2017 17:30:36 +0100
> > > 
> > > Move two debug messages so that a null pointer access can not happen
> > > for the variable "ch" in these functions.
> > 
> > An actual defect fix!
> 
> Nope, not at all, this does not "fix" anything.

Well, I believe it does in unusual cases like a
CONFIG_DYNAMIC_DEBUG when this is enabled by an
odd +p in the dynamic debug control file.

> > Here you could probably cc stable too.
> 
> Nope, not worth it.

<shrug>

It's pretty unlikely, but it is an actual defect.

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Greg Kroah-Hartman]

On Wed, Nov 29, 2017 at 09:51:36AM -0800, Joe Perches wrote:
> On Wed, 2017-11-29 at 17:35 +0000, Greg Kroah-Hartman wrote:
> > On Wed, Nov 29, 2017 at 09:23:07AM -0800, Joe Perches wrote:
> > > On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> > > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > > Date: Wed, 29 Nov 2017 17:30:36 +0100
> > > > 
> > > > Move two debug messages so that a null pointer access can not happen
> > > > for the variable "ch" in these functions.
> > > 
> > > An actual defect fix!
> > 
> > Nope, not at all, this does not "fix" anything.
> 
> Well, I believe it does in unusual cases like a
> CONFIG_DYNAMIC_DEBUG when this is enabled by an
> odd +p in the dynamic debug control file.
> 
> > > Here you could probably cc stable too.
> > 
> > Nope, not worth it.
> 
> <shrug>
> 
> It's pretty unlikely, but it is an actual defect.

No it is not, those variables will never be set to NULL, so this can
never be triggered.  Walk up the call chain.

greg k-h

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Joe Perches]

On Wed, 2017-11-29 at 18:05 +0000, Greg Kroah-Hartman wrote:
> On Wed, Nov 29, 2017 at 09:51:36AM -0800, Joe Perches wrote:
> > On Wed, 2017-11-29 at 17:35 +0000, Greg Kroah-Hartman wrote:
> > > On Wed, Nov 29, 2017 at 09:23:07AM -0800, Joe Perches wrote:
> > > > On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> > > > > From: Markus Elfring <elfring@users.sourceforge.net>
> > > > > Date: Wed, 29 Nov 2017 17:30:36 +0100
> > > > > 
> > > > > Move two debug messages so that a null pointer access can not happen
> > > > > for the variable "ch" in these functions.
> > > > 
> > > > An actual defect fix!
> > > 
> > > Nope, not at all, this does not "fix" anything.
> > 
> > Well, I believe it does in unusual cases like a
> > CONFIG_DYNAMIC_DEBUG when this is enabled by an
> > odd +p in the dynamic debug control file.
> > 
> > > > Here you could probably cc stable too.
> > > 
> > > Nope, not worth it.
> > 
> > <shrug>
> > 
> > It's pretty unlikely, but it is an actual defect.
> 
> No it is not, those variables will never be set to NULL, so this can
> never be triggered.  Walk up the call chain.

Right you are.  Local analysis isn't enough.
The code could/should be removed, but it's not a defect.

cheers, Joe

Re: jsm_tty: Fix a possible null pointer dereference in two functions

[SF Markus Elfring]

>> It's pretty unlikely, but it is an actual defect.
> 
> No it is not, those variables will never be set to NULL,
> so this can never be triggered.  Walk up the call chain.

If the involved software developers are convinced about the validity
of this pointer:

How do you think about to delete the following condition check
instead in the discussed function implementations?

	if (!ch)
		return;


Regards,
Markus

Re: [PATCH] jsm_tty: Fix a possible null pointer dereference in two functions

[Dan Carpenter]

On Wed, Nov 29, 2017 at 09:23:07AM -0800, Joe Perches wrote:
> On Wed, 2017-11-29 at 17:40 +0100, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 29 Nov 2017 17:30:36 +0100
> > 
> > Move two debug messages so that a null pointer access can not happen
> > for the variable "ch" in these functions.
> 
> An actual defect fix!
> 
> Here you could probably cc stable too.
> 

These aren't real bugs because "ch" is never NULL.

regards,
dan carpenter

Re: jsm_tty: Fix a possible null pointer dereference in two functions

[Jiri Slaby]

On 11/29/2017, 07:19 PM, SF Markus Elfring wrote:
>>> It's pretty unlikely, but it is an actual defect.
>>
>> No it is not, those variables will never be set to NULL,
>> so this can never be triggered.  Walk up the call chain.
> 
> If the involved software developers are convinced about the validity
> of this pointer:
> 
> How do you think about to delete the following condition check
> instead in the discussed function implementations?
> 
> 	if (!ch)
> 		return;

ACK

thanks,
-- 
js
suse labs

Re: jsm_tty: Fix a possible null pointer dereference in two functions

[Guilherme G. Piccoli]

On 11/29/2017 04:19 PM, SF Markus Elfring wrote:
>>> It's pretty unlikely, but it is an actual defect.
>>
>> No it is not, those variables will never be set to NULL,
>> so this can never be triggered.  Walk up the call chain.
> 
> If the involved software developers are convinced about the validity
> of this pointer:
> 
> How do you think about to delete the following condition check
> instead in the discussed function implementations?
> 
> 	if (!ch)
> 		return;
> 
> 
> Regards,
> Markus
> 

Thanks for the fix.
I was on vacation - but now seeing all the analysis made here, if "ch"
can't be NULL then please go ahead and remove the check =)
I observed that this check comes from before Git, so really ancient code...

Cheers,


Guilherme

Re: jsm_tty: Deletion of a null pointer check in two functions?

[SF Markus Elfring]

> Thanks for the fix.

Thanks for your positive feedback.


> I was on vacation - but now seeing all the analysis made here,

I assume that special communication settings could trigger
corresponding consequences for the discussed source code adjustment.


> if "ch" can't be NULL then please go ahead and remove the check =)

Would you dare to convert this request into a concrete patch?

Regards,
Markus

Re: jsm_tty: Deletion of a null pointer check in two functions?

[Guilherme G. Piccoli]

On 12/16/2017 04:27 AM, SF Markus Elfring wrote:
>> Thanks for the fix.
> 
> Thanks for your positive feedback.
> 
> 
>> I was on vacation - but now seeing all the analysis made here,
> 
> I assume that special communication settings could trigger
> corresponding consequences for the discussed source code adjustment.
> 
> 
>> if "ch" can't be NULL then please go ahead and remove the check =)
> 
> Would you dare to convert this request into a concrete patch?

For me, it's OK if you send the patch or I can do it myself.
It's a minor patch...but it's correct the way I see it.

So, what do you prefer? Send it yourself, or want me to send
it with your sign-off too? (since was your idea).

Thanks,


Guilherme


> 
> Regards,
> Markus
> 

Re: jsm_tty: Deletion of a null pointer check in two functions?

[SF Markus Elfring]

> So, what do you prefer? Send it yourself,

I became concerned that “my patch” would not get picked up because of
Greg's communication settings (for my initial messages when they do not get
resent by other contributors).


> or want me to send it with your sign-off too? (since was your idea).

I would prefer this variant because I assume that the chances for integration
are higher for the discussed small source code adjustment.
I guess that the tag “Suggested-by” would be sufficient in this case.

Regards,
Markus