[PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Guillaume Juan]

From: Guillaume Juan <guillaume.juan@sagemcom.com>

If gsm->tty happens to be NULL in gsmld_output, avoid crashing the kernel (the crash is replaced by a warning dump).
Prevent at earlier level such situation:
- gsmtty_hangup does no longer call gsm_dlci_begin_close when called synchronously from gsm_cleanup_mux, because this would arm a timer that will asynchronously lead to use gms->tty, potentially after it is nullified.
- in gsm_cleanup_mux, release DLCIs (other than DLCI#0 which is the control one) before closing mux. Else, it can happen that the T2 timer is rearmed by another thread scheduled between del_timer_sync and gsm_dlci release
(for example by the following call-stack: fput => tty_release => tty_port_close_start => tty_port_lower_dtr_rts => gsm_dtr_rts => gsmtty_modem_update)

Signed-off-by: Guillaume Juan <guillaume.juan@sagemcom.com>
---
 drivers/tty/n_gsm.c |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 1e8e8ce..fe3c6ad 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -1510,8 +1510,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
 }
 
 /**
- *	gsm_dlci_begin_close	-	start channel open procedure
- *	@dlci: DLCI to open
+ *	gsm_dlci_begin_close	-	start channel close procedure
+ *	@dlci: DLCI to close
  *
  *	Commence closing a DLCI from the Linux side. We issue DISC messages
  *	to the modem which should then reply with a UA, at which point we
@@ -2031,6 +2031,13 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
 	spin_unlock(&gsm_mux_lock);
 	WARN_ON(i == MAX_MUX);
 
+	/* Free up any link layer users */
+	if (dlci)
+		dlci->dead = 1;
+	for (i = 1; i < NUM_DLCI; i++)
+		if (gsm->dlci[i])
+			gsm_dlci_release(gsm->dlci[i]);
+
 	/* In theory disconnecting DLCI 0 is sufficient but for some
 	   modems this is apparently not the case. */
 	if (dlci) {
@@ -2041,15 +2048,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
 	del_timer_sync(&gsm->t2_timer);
 	/* Now we are sure T2 has stopped */
 	if (dlci) {
-		dlci->dead = 1;
 		gsm_dlci_begin_close(dlci);
 		wait_event_interruptible(gsm->event,
 					dlci->state == DLCI_CLOSED);
+		gsm_dlci_release(dlci);
 	}
-	/* Free up any link layer users */
-	for (i = 0; i < NUM_DLCI; i++)
-		if (gsm->dlci[i])
-			gsm_dlci_release(gsm->dlci[i]);
 	/* Now wipe the queues */
 	list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)
 		kfree(txq);
@@ -2192,6 +2195,10 @@ EXPORT_SYMBOL_GPL(gsm_alloc_mux);
 
 static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
 {
+	if (gsm->tty == NULL) {
+		WARN(1, "NULL gsm->tty pointer in gsmld_output\n");
+		return -EINVAL;
+	}
 	if (tty_write_room(gsm->tty) < len) {
 		set_bit(TTY_DO_WRITE_WAKEUP, &gsm->tty->flags);
 		return -ENOSPC;
@@ -2323,7 +2330,7 @@ static void gsmld_flush_buffer(struct tty_struct *tty)
  *	@tty: device
  *
  *	Called from the terminal layer when this line discipline is
- *	being shut down, either because of a close or becsuse of a
+ *	being shut down, either because of a close or because of a
  *	discipline change. The function will not be called while other
  *	ldisc methods are in progress.
  */
@@ -2954,6 +2961,15 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	gsm = dlci->gsm;
 	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
 		goto out;
+	/* Should not happen because the DLCI ttys are hung up (which causes
+	   tty_port_close_start to return 0) by gsm_dlci_release before setting
+	   gsm->tty to NULL */
+	if (gsm->tty == NULL) {
+		WARN(1, "NULL gsm->tty pointer in gsmtty_close for %s\n",
+			tty->name);
+		goto out;
+	}
+
 	gsm_dlci_begin_close(dlci);
 	tty_port_close_end(&dlci->port, tty);
 	tty_port_tty_set(&dlci->port, NULL);
@@ -2967,7 +2983,8 @@ static void gsmtty_hangup(struct tty_struct *tty)
 {
 	struct gsm_dlci *dlci = tty->driver_data;
 	tty_port_hangup(&dlci->port);
-	gsm_dlci_begin_close(dlci);
+	if (!dlci->gsm->dead)
+		gsm_dlci_begin_close(dlci);
 }
 
 static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
-- 
1.7.0.4



#
" Ce courriel et les documents qui lui sont joints peuvent contenir des
informations confidentielles ou ayant un caractère privé. S'ils ne vous sont
pas destinés, nous vous signalons qu'il est strictement interdit de les
divulguer, de les reproduire ou d'en utiliser de quelque manière que ce
soit le contenu. Si ce message vous a été transmis par erreur, merci d'en
informer l'expéditeur et de supprimer immédiatement de votre système
informatique ce courriel ainsi que tous les documents qui y sont attachés."


                               ******

" This e-mail and any attached documents may contain confidential or
proprietary information. If you are not the intended recipient, you are
notified that any dissemination, copying of this e-mail and any attachments
thereto or use of their contents by any means whatsoever is strictly
prohibited. If you have received this e-mail in error, please advise the
sender immediately and delete this e-mail and all attached documents
from your computer system."
#

Re: [PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Greg Kroah-Hartman]

On Fri, Oct 26, 2012 at 10:11:45AM +0200, Guillaume Juan wrote:
> From: Guillaume Juan <guillaume.juan@sagemcom.com>
> 
> If gsm->tty happens to be NULL in gsmld_output, avoid crashing the kernel (the crash is replaced by a warning dump).

How can ->tty be NULL here?

> Prevent at earlier level such situation:
> - gsmtty_hangup does no longer call gsm_dlci_begin_close when called synchronously from gsm_cleanup_mux, because this would arm a timer that will asynchronously lead to use gms->tty, potentially after it is nullified.
> - in gsm_cleanup_mux, release DLCIs (other than DLCI#0 which is the control one) before closing mux. Else, it can happen that the T2 timer is rearmed by another thread scheduled between del_timer_sync and gsm_dlci release
> (for example by the following call-stack: fput => tty_release => tty_port_close_start => tty_port_lower_dtr_rts => gsm_dtr_rts => gsmtty_modem_update)

Please wrap your changelog comments at 72 columns.

> 
> Signed-off-by: Guillaume Juan <guillaume.juan@sagemcom.com>
> ---
>  drivers/tty/n_gsm.c |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 1e8e8ce..fe3c6ad 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -1510,8 +1510,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>  }
>  
>  /**
> - *	gsm_dlci_begin_close	-	start channel open procedure
> - *	@dlci: DLCI to open
> + *	gsm_dlci_begin_close	-	start channel close procedure
> + *	@dlci: DLCI to close
>   *
>   *	Commence closing a DLCI from the Linux side. We issue DISC messages
>   *	to the modem which should then reply with a UA, at which point we
> @@ -2031,6 +2031,13 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
>  	spin_unlock(&gsm_mux_lock);
>  	WARN_ON(i == MAX_MUX);
>  
> +	/* Free up any link layer users */
> +	if (dlci)
> +		dlci->dead = 1;
> +	for (i = 1; i < NUM_DLCI; i++)
> +		if (gsm->dlci[i])
> +			gsm_dlci_release(gsm->dlci[i]);
> +
>  	/* In theory disconnecting DLCI 0 is sufficient but for some
>  	   modems this is apparently not the case. */
>  	if (dlci) {
> @@ -2041,15 +2048,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
>  	del_timer_sync(&gsm->t2_timer);
>  	/* Now we are sure T2 has stopped */
>  	if (dlci) {
> -		dlci->dead = 1;
>  		gsm_dlci_begin_close(dlci);
>  		wait_event_interruptible(gsm->event,
>  					dlci->state == DLCI_CLOSED);
> +		gsm_dlci_release(dlci);
>  	}
> -	/* Free up any link layer users */
> -	for (i = 0; i < NUM_DLCI; i++)
> -		if (gsm->dlci[i])
> -			gsm_dlci_release(gsm->dlci[i]);
>  	/* Now wipe the queues */
>  	list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)
>  		kfree(txq);
> @@ -2192,6 +2195,10 @@ EXPORT_SYMBOL_GPL(gsm_alloc_mux);
>  
>  static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
>  {
> +	if (gsm->tty == NULL) {
> +		WARN(1, "NULL gsm->tty pointer in gsmld_output\n");
> +		return -EINVAL;
> +	}
>  	if (tty_write_room(gsm->tty) < len) {
>  		set_bit(TTY_DO_WRITE_WAKEUP, &gsm->tty->flags);
>  		return -ENOSPC;
> @@ -2323,7 +2330,7 @@ static void gsmld_flush_buffer(struct tty_struct *tty)
>   *	@tty: device
>   *
>   *	Called from the terminal layer when this line discipline is
> - *	being shut down, either because of a close or becsuse of a
> + *	being shut down, either because of a close or because of a
>   *	discipline change. The function will not be called while other
>   *	ldisc methods are in progress.
>   */
> @@ -2954,6 +2961,15 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
>  	gsm = dlci->gsm;
>  	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
>  		goto out;
> +	/* Should not happen because the DLCI ttys are hung up (which causes
> +	   tty_port_close_start to return 0) by gsm_dlci_release before setting
> +	   gsm->tty to NULL */
> +	if (gsm->tty == NULL) {
> +		WARN(1, "NULL gsm->tty pointer in gsmtty_close for %s\n",
> +			tty->name);
> +		goto out;
> +	}
> +
>  	gsm_dlci_begin_close(dlci);
>  	tty_port_close_end(&dlci->port, tty);
>  	tty_port_tty_set(&dlci->port, NULL);
> @@ -2967,7 +2983,8 @@ static void gsmtty_hangup(struct tty_struct *tty)
>  {
>  	struct gsm_dlci *dlci = tty->driver_data;
>  	tty_port_hangup(&dlci->port);
> -	gsm_dlci_begin_close(dlci);
> +	if (!dlci->gsm->dead)
> +		gsm_dlci_begin_close(dlci);
>  }
>  
>  static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
> -- 
> 1.7.0.4
> 
> 
> 
> #
> " Ce courriel et les documents qui lui sont joints peuvent contenir des
> informations confidentielles ou ayant un caractère privé. S'ils ne vous sont
> pas destinés, nous vous signalons qu'il est strictement interdit de les
> divulguer, de les reproduire ou d'en utiliser de quelque manière que ce
> soit le contenu. Si ce message vous a été transmis par erreur, merci d'en
> informer l'expéditeur et de supprimer immédiatement de votre système
> informatique ce courriel ainsi que tous les documents qui y sont attachés."
> 
> 
>                                ******
> 
> " This e-mail and any attached documents may contain confidential or
> proprietary information. If you are not the intended recipient, you are
> notified that any dissemination, copying of this e-mail and any attachments
> thereto or use of their contents by any means whatsoever is strictly
> prohibited. If you have received this e-mail in error, please advise the
> sender immediately and delete this e-mail and all attached documents
> from your computer system."
> #

Because of that footer, I am not allowed to accept this patch at all.
Please remove it and resend.

thanks,

greg k-h

Re: [PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Guillaume Juan]

Le 26/10/2012 17:20, Greg Kroah-Hartman a écrit :

> On Fri, Oct 26, 2012 at 10:11:45AM +0200, Guillaume Juan wrote:
>> From: Guillaume Juan <guillaume.juan@sagemcom.com>
>>
>> If gsm->tty happens to be NULL in gsmld_output, avoid crashing the kernel (the crash is replaced by a warning dump).
> 
> How can ->tty be NULL here?
> 

I had some crashes and identified 2 causes: this is what I tried to explain in the 2nd part of the changelog ("Prevent at earlier level such situation: [...]"). Basically it is because the T1 and T2 timers may be rearmed while gsm_cleanup_mux executes (T1 is rearmed by gsmtty_hangup called by gsm_cleanup_mux itself via gsm_dlci_release, whereas T2 may be rearmed by a concurrent call to gsmtty_close).

My patch is intended to remove theses causes, but I thought safer to avoid the crash in gsmld_output if ever there are other causes remaining. Do you mean I should let the crash happen for these unproven cases?

>> Prevent at earlier level such situation:
>> - gsmtty_hangup does no longer call gsm_dlci_begin_close when called synchronously from gsm_cleanup_mux, because this would arm a timer that will asynchronously lead to use gms->tty, potentially after it is nullified.
>> - in gsm_cleanup_mux, release DLCIs (other than DLCI#0 which is the control one) before closing mux. Else, it can happen that the T2 timer is rearmed by another thread scheduled between del_timer_sync and gsm_dlci release
>> (for example by the following call-stack: fput => tty_release => tty_port_close_start => tty_port_lower_dtr_rts => gsm_dtr_rts => gsmtty_modem_update)
> 
> Please wrap your changelog comments at 72 columns.
>

Sorry, I didn't know this rule (it is the 1st time I post a patch and the 1st time I use git). I will when I re-send.


>>
>> Signed-off-by: Guillaume Juan <guillaume.juan@sagemcom.com>
>> ---
>>  drivers/tty/n_gsm.c |   35 ++++++++++++++++++++++++++---------
>>  1 files changed, 26 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
>> index 1e8e8ce..fe3c6ad 100644
>> --- a/drivers/tty/n_gsm.c
>> +++ b/drivers/tty/n_gsm.c
>> @@ -1510,8 +1510,8 @@ static void gsm_dlci_begin_open(struct gsm_dlci *dlci)
>>  }
>>  
>>  /**
>> - *	gsm_dlci_begin_close	-	start channel open procedure
>> - *	@dlci: DLCI to open
>> + *	gsm_dlci_begin_close	-	start channel close procedure
>> + *	@dlci: DLCI to close
>>   *
>>   *	Commence closing a DLCI from the Linux side. We issue DISC messages
>>   *	to the modem which should then reply with a UA, at which point we
>> @@ -2031,6 +2031,13 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
>>  	spin_unlock(&gsm_mux_lock);
>>  	WARN_ON(i == MAX_MUX);
>>  
>> +	/* Free up any link layer users */
>> +	if (dlci)
>> +		dlci->dead = 1;
>> +	for (i = 1; i < NUM_DLCI; i++)
>> +		if (gsm->dlci[i])
>> +			gsm_dlci_release(gsm->dlci[i]);
>> +
>>  	/* In theory disconnecting DLCI 0 is sufficient but for some
>>  	   modems this is apparently not the case. */
>>  	if (dlci) {
>> @@ -2041,15 +2048,11 @@ void gsm_cleanup_mux(struct gsm_mux *gsm)
>>  	del_timer_sync(&gsm->t2_timer);
>>  	/* Now we are sure T2 has stopped */
>>  	if (dlci) {
>> -		dlci->dead = 1;
>>  		gsm_dlci_begin_close(dlci);
>>  		wait_event_interruptible(gsm->event,
>>  					dlci->state == DLCI_CLOSED);
>> +		gsm_dlci_release(dlci);
>>  	}
>> -	/* Free up any link layer users */
>> -	for (i = 0; i < NUM_DLCI; i++)
>> -		if (gsm->dlci[i])
>> -			gsm_dlci_release(gsm->dlci[i]);
>>  	/* Now wipe the queues */
>>  	list_for_each_entry_safe(txq, ntxq, &gsm->tx_list, list)
>>  		kfree(txq);
>> @@ -2192,6 +2195,10 @@ EXPORT_SYMBOL_GPL(gsm_alloc_mux);
>>  
>>  static int gsmld_output(struct gsm_mux *gsm, u8 *data, int len)
>>  {
>> +	if (gsm->tty == NULL) {
>> +		WARN(1, "NULL gsm->tty pointer in gsmld_output\n");
>> +		return -EINVAL;
>> +	}
>>  	if (tty_write_room(gsm->tty) < len) {
>>  		set_bit(TTY_DO_WRITE_WAKEUP, &gsm->tty->flags);
>>  		return -ENOSPC;
>> @@ -2323,7 +2330,7 @@ static void gsmld_flush_buffer(struct tty_struct *tty)
>>   *	@tty: device
>>   *
>>   *	Called from the terminal layer when this line discipline is
>> - *	being shut down, either because of a close or becsuse of a
>> + *	being shut down, either because of a close or because of a
>>   *	discipline change. The function will not be called while other
>>   *	ldisc methods are in progress.
>>   */
>> @@ -2954,6 +2961,15 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
>>  	gsm = dlci->gsm;
>>  	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
>>  		goto out;
>> +	/* Should not happen because the DLCI ttys are hung up (which causes
>> +	   tty_port_close_start to return 0) by gsm_dlci_release before setting
>> +	   gsm->tty to NULL */
>> +	if (gsm->tty == NULL) {
>> +		WARN(1, "NULL gsm->tty pointer in gsmtty_close for %s\n",
>> +			tty->name);
>> +		goto out;
>> +	}
>> +
>>  	gsm_dlci_begin_close(dlci);
>>  	tty_port_close_end(&dlci->port, tty);
>>  	tty_port_tty_set(&dlci->port, NULL);
>> @@ -2967,7 +2983,8 @@ static void gsmtty_hangup(struct tty_struct *tty)
>>  {
>>  	struct gsm_dlci *dlci = tty->driver_data;
>>  	tty_port_hangup(&dlci->port);
>> -	gsm_dlci_begin_close(dlci);
>> +	if (!dlci->gsm->dead)
>> +		gsm_dlci_begin_close(dlci);
>>  }
>>  
>>  static int gsmtty_write(struct tty_struct *tty, const unsigned char *buf,
>> -- 
>> 1.7.0.4
>>
>>
>>
>> #
>> " Ce courriel et les documents qui lui sont joints peuvent contenir des
>> informations confidentielles ou ayant un caractère privé. S'ils ne vous sont
>> pas destinés, nous vous signalons qu'il est strictement interdit de les
>> divulguer, de les reproduire ou d'en utiliser de quelque manière que ce
>> soit le contenu. Si ce message vous a été transmis par erreur, merci d'en
>> informer l'expéditeur et de supprimer immédiatement de votre système
>> informatique ce courriel ainsi que tous les documents qui y sont attachés."
>>
>>
>>                                ******
>>
>> " This e-mail and any attached documents may contain confidential or
>> proprietary information. If you are not the intended recipient, you are
>> notified that any dissemination, copying of this e-mail and any attachments
>> thereto or use of their contents by any means whatsoever is strictly
>> prohibited. If you have received this e-mail in error, please advise the
>> sender immediately and delete this e-mail and all attached documents
>> from your computer system."
>> #
> 
> Because of that footer, I am not allowed to accept this patch at all.
> Please remove it and resend.
> 
> thanks,
> 
> greg k-h


Argh. I'm afraid I don't know how to remove it, since it seems added automatically by my company mail server. I probably will have to struggle with the admins. But since you ARE the "intended recipient" and the content is NOT "confidential or proprietary", does it really forbid you to use the patch?

#
" Ce courriel et les documents qui lui sont joints peuvent contenir des
informations confidentielles ou ayant un caractère privé. S'ils ne vous sont
pas destinés, nous vous signalons qu'il est strictement interdit de les
divulguer, de les reproduire ou d'en utiliser de quelque manière que ce
soit le contenu. Si ce message vous a été transmis par erreur, merci d'en
informer l'expéditeur et de supprimer immédiatement de votre système
informatique ce courriel ainsi que tous les documents qui y sont attachés."


                               ******

" This e-mail and any attached documents may contain confidential or
proprietary information. If you are not the intended recipient, you are
notified that any dissemination, copying of this e-mail and any attachments
thereto or use of their contents by any means whatsoever is strictly
prohibited. If you have received this e-mail in error, please advise the
sender immediately and delete this e-mail and all attached documents
from your computer system."
#

Re: [PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Greg Kroah-Hartman]

On Fri, Oct 26, 2012 at 06:34:23PM +0200, Guillaume Juan wrote:
> Le 26/10/2012 17:20, Greg Kroah-Hartman a écrit :
> 
> > On Fri, Oct 26, 2012 at 10:11:45AM +0200, Guillaume Juan wrote:
> >> From: Guillaume Juan <guillaume.juan@sagemcom.com>
> >>
> >> If gsm->tty happens to be NULL in gsmld_output, avoid crashing the kernel (the crash is replaced by a warning dump).
> > 
> > How can ->tty be NULL here?
> > 
> 
> I had some crashes and identified 2 causes: this is what I tried to explain in the 2nd part of the changelog ("Prevent at earlier level such situation: [...]"). Basically it is because the T1 and T2 timers may be rearmed while gsm_cleanup_mux executes (T1 is rearmed by gsmtty_hangup called by gsm_cleanup_mux itself via gsm_dlci_release, whereas T2 may be rearmed by a concurrent call to gsmtty_close).
> 
> My patch is intended to remove theses causes, but I thought safer to avoid the crash in gsmld_output if ever there are other causes remaining. Do you mean I should let the crash happen for these unproven cases?

If you leave it, and it happens, then you know you didn't really fix the
real problem :)

> >> " This e-mail and any attached documents may contain confidential or
> >> proprietary information. If you are not the intended recipient, you are
> >> notified that any dissemination, copying of this e-mail and any attachments
> >> thereto or use of their contents by any means whatsoever is strictly
> >> prohibited. If you have received this e-mail in error, please advise the
> >> sender immediately and delete this e-mail and all attached documents
> >> from your computer system."
> >> #
> > 
> > Because of that footer, I am not allowed to accept this patch at all.
> > Please remove it and resend.
> 
> Argh. I'm afraid I don't know how to remove it, since it seems added automatically by my company mail server. I probably will have to struggle with the admins. But since you ARE the "intended recipient" and the content is NOT "confidential or proprietary", does it really forbid you to use the patch?

Yes it does, it goes against the "Signed-off-by:" line in the patch, and
because of that, I am not allowed to accept it.  If you wish to do
kernel development, either get your email admins to fix it, or use an
external email account (sadly the only solution for a lot of people.)
If you use an external account, be sure you properly set the "From:"
line in the patch to your properly account, see the file,
Documentation/SubmittingPatches for details.

thanks,

greg k-h

Re: [PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Alan Cox]

On Fri, 26 Oct 2012 10:11:45 +0200
Guillaume Juan <guillaume.juan@sagemcom.com> wrote:

> From: Guillaume Juan <guillaume.juan@sagemcom.com>
> 
> If gsm->tty happens to be NULL in gsmld_output, avoid crashing the kernel (the crash is replaced by a warning dump).

More important is fixing the bug completely. I agree there is a bug I
don't think your fix is sufficient however.


You no longer set dlci->dead before doing the dlci release so what stops
a SABM from the other end racing this ?

Also moving the gsm_dlci_release seems to have no value at all because
there is no locking in the code concerned so viewed from any other thread
you've changed nothing but timings. Yes its probably way harder to hit.

I can see two ways of tackling it both of which basically come down to
the same thing. In gsmld_detach_gsm after the gsm_cleanup_mux we need to

- be sure the thing cannot re-open
- wait until all the DLCIs are dead

then do the tty_kref_put and gsm->tty = NULL

Thoughts ?

Alan

Re: [PATCH] n_gsm: prevent crash due to dereferencing NULL gsm->tty

[Guillaume Juan]

Le 29/10/2012 17:29, Alan Cox a écrit :

> 
> More important is fixing the bug completely. I agree there is a bug I
> don't think your fix is sufficient however.
>

It may not fix all cases but I think it improves things both from a
practical and theoretical point of view.
In particular the part in gsmtty_hangup: the crash was systematic if the
user space released the virtual ttys fds only after the N_GSM line
discipline was removed (with ioctl).
I explained all the crashes I had, and did not reproduced crashes after
my patch. Note that I don't have use-cases where the DCE itself opens
some DLCIs, which certainly can cause a lot more trouble.


> 
> You no longer set dlci->dead before doing the dlci release

Yes I do, see line 2036:
+	if (dlci)
+		dlci->dead = 1;
I do it only for DLCI#0 but this is the same as what the previous code
was doing (I don't understand why but I thought that was another topic).
I was not sure that this dlci->dead was really efficient, but I decided
to keep it because addressing the relevance of dlci->dead was not the
purpose of my patch.

> Also moving the gsm_dlci_release seems to have no value at all because
> there is no locking in the code concerned so viewed from any other thread
> you've changed nothing but timings. Yes its probably way harder to hit.
> 

I don't really see your point, or I think you may be speaking of a
different matter. Once gsm_dlci_release has done its job, the T2 timer
should no longer be rearmed by anything the user-space could do, because
the tty will be hung up (tty_vhangup is synchronous isn't it ?). At
least tty_port_close_start would not call tty_port_lower_dtr_rts
anymore. So then the comment /* Now we are sure T2 has stopped */ after
del_timer_sync will be true.
Do you see any other paths that may lead to relaunch the T2 timer
afterwards? Don't you agree that gsmtty_release should be done before
del_timer_sync?

> I can see two ways of tackling it both of which basically come down to
> the same thing. In gsmld_detach_gsm after the gsm_cleanup_mux we need to
> 
> - be sure the thing cannot re-open

If you talk about reopening from the other side, I agree with you that
dlci->dead does not seem to protect enough. However I can read in the
description of gsmld_close that "the function will not be called while
other ldisc methods are in progress". If this is true, the concurrence
of gsmld_receive_buf should not be possible ?

> - wait until all the DLCIs are dead

I am not sure what you mean by this:
- gsm_cleanup_mux already waits for the answer to CLD command and to the
closure of DLCI#0. As I reported previously, it will never see the
answers coming because of the line-discipline life-cycle design, but
this is yet another topic.
- or maybe you mean wait until we are sure there are no longer
references to the virtual ttys. This seems to me impossible to achieve
because the user-space closes them at its own pace.

> then do the tty_kref_put and gsm->tty = NULL
> 
> Thoughts ?
> 
> Alan


Guillaume

#
" Ce courriel et les documents qui lui sont joints peuvent contenir des
informations confidentielles ou ayant un caractère privé. S'ils ne vous sont
pas destinés, nous vous signalons qu'il est strictement interdit de les
divulguer, de les reproduire ou d'en utiliser de quelque manière que ce
soit le contenu. Si ce message vous a été transmis par erreur, merci d'en
informer l'expéditeur et de supprimer immédiatement de votre système
informatique ce courriel ainsi que tous les documents qui y sont attachés."


                               ******

" This e-mail and any attached documents may contain confidential or
proprietary information. If you are not the intended recipient, you are
notified that any dissemination, copying of this e-mail and any attachments
thereto or use of their contents by any means whatsoever is strictly
prohibited. If you have received this e-mail in error, please advise the
sender immediately and delete this e-mail and all attached documents
from your computer system."
#