Re: PATCH against 2.4.2: TTY hangup on PPP channel corrupts kernel memory

Paul Mackerras (paulus@samba.org)
Fri, 23 Mar 2001 15:34:55 +1100 (EST)


Kevin Buhr writes:

> I didn't realize my specific hang was a peculiarity of the older
> attachment style. The channel created by pushing the PPP line

I didn't realize you were talking about linux 2.4.0 and pppd 2.3.11.

> discipline onto a TTY was connected to a unit with a PPPIOCATTACH
> ioctl on the TTY---this didn't really "attach" the channel; it still
> had a refcnt of only one. Through the old compatibility interface, it
> was possible to call ppp_asynctty_read -> ppp_channel_read -> ppp_read
> on the channel's "struct ppp_file" and wait on the channel's "rwait".
> If the modem hung up, "do_tty_hangup" would call "ppp_asynctty_close"
> (with a reader still in "ppp_asynctty_read") and the "struct channel"
> would be freed in "ppp_unregister_channel".

That's one of the main reasons why I removed the compatibility
stuff. :)

> I think your analysis of how things presently are with 2.4.2 and a
> modern "pppd" is correct...
>
> Since the new "pppd" uses an explicit PPPIOCATTCHAN / PPPIOCCONNECT
> sequence, the refcnt gets bumped to 2 and stays there while the
> channel is attached. So, this specific hang isn't a problem anymore
> for "ppp_async.c". It's still a problem with "ppp_synctty.c", though
> (when used with "pppd" 2.3.11, say). Is the compatibility stuff in
> there slated for removal, too?

Yep, and we should take out the stuff in ppp_generic.c that was called
by the compatibility stuff in the channels, too.

> In particular, the comment above "ppp_asynctty_close" is misleading.
> It's true that the TTY layer won't call any further line discipline
> entries while the "close" is executing; however, there may be
> processes already sleeping in line discipline functions called before
> the hangup. For example, "ppp_asynctty_close" could be called while
> we sleep in the "get_user" in "ppp_channel_ioctl" (called from
> "ppp_asynctty_ioctl"). Therefore, calling "PPPIOCATTACH" on an
> unattached PPP-disciplined TTY could, in unlikely circumstances
> (argument swapped out), lead to a crash.

Yuck. I don't see that we can protect against this without having
some sort of lock in the tty structure, though. We can't protect the
existence of the channel structure with a lock inside that structure.
Ideally the necessary protection would be provided at the tty level.

> I assume PPPIOCATTACH (on the TTY) is deprecated in favor of
> PPPIOCATTCHAN / PPPIOCCONNECT (on the "/dev/ppp" handle). Can we
> eliminate "ppp_channel_ioctl" from "ppp_async.c" entirely, as in the
> patch below? We're requiring people to upgrade to "pppd" 2.4.0
> anyway, and it has no need for these calls. This would give me a warm,
> fuzzy feeling.

Sure, that would be fine. I'll make up a patch and send it to Linus.

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