> We think either (1) gsm_dlci_alloc() should hold a lock(mutex) and do > internal check about whether gsm->dlci[addr] is NUll or not, OR > (2) all callers of gsm_dlci_alloc() should hold gsm->mutex and check > whether gsm->dlci[addr] is NUll or not (like gsmtty_install()). > > Could you check this? If it makes sense, we will write a patch > following one of the suggestions. We are currently dealing with multiple race conditions in the n_gsm. Not only for the syzkaller reports but in recent exploits for example, too. I believe we need an overall concept/solution for these. Currently, the following actors can race: - ioctl (e.g. resetting the mux or one of its DLCIs) - ldisc callbacks (e.g. receiving SABM or DISC in gsm_queue()) - tty callbacks (e.g. gsmtty_hangup()) - internal write task (gsmld_write_task()) - internal timers (e.g. gsm_control_keep_alive()) - driver removal Each with another and ioctl's from multiple threads. Guarding these is not trivial for the following reasons: - execution context may not allow sleep (timers, write task, tty callbacks?) - creating an atomic section may conflict inner sleeps (e.g. ioctl) - dead-locking needs to be considered - locking may introduce high performance bottlenecks Still, not guarding one of the racing actors does not appears to be possible as I see it. Unfortunately, for the same reason the sleep while atomic issue when using a console on a virtual tty has not been fixed yet, I have no solution at hand. We are dealing with a quite complex situation here. Nevertheless, creating many small patches here and there should not be our solution for obvious reasons. This does not give a complete solution and makes it harder to find the remaining corner cases. I can assist to find a solution here, but I have not enough time to do this alone at the moment. Best regards, Daniel Starke
