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."
#

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/