>>>>> "Michal" == Michal Ostrowski <mostrows@xxxxxxxxxxxxxxxxx> writes:
Michal> Comments, feedback, patches and bug reports are welcome
Michal> and encouraged.
o.k.. I did not run it, just tried to look at the code. Thus, I did no
verify whether the problems identified theoretically are really exploitable
and I might be wrong with some comments.
Anyway, the code looks clean to me and is fairly easy to follow.
>
>diff -r -N -u linux.orig/drivers/net/Config.in linux/drivers/net/Config.in
>--- linux.orig/drivers/net/Config.in Sat Apr 15 15:58:21 2000
>+++ linux/drivers/net/Config.in Sat Apr 15 08:21:15 2000
> ^^^^^^^^^^^
As this is implemented as a new protocol family, I think it should be moved
somewhere in network protocol directory, e.g. linux/net/pppox/.
>diff -r -N -u linux.orig/drivers/net/ppp_generic.c
>linux/drivers/net/ppp_generic.c
>--- linux.orig/drivers/net/ppp_generic.c Sat Apr 15 15:58:21 2000
>+++ linux/drivers/net/ppp_generic.c Sun Apr 16 18:55:48 2000
>@@ -2382,14 +2382,16 @@
> if (ppp != 0) {
> /* remove it from the ppp unit's list */
> pch->ppp = NULL;
>- ppp_lock(ppp);
>+ ppp_lock(ppp); /* This disables bh twice */
> list_del(&pch->clist);
> --ppp->n_channels;
>- if (ppp->dev == 0 && ppp->n_channels == 0)
>+ if (ppp->dev == 0 && ppp->n_channels == 0){
> /* Last disconnect from a ppp unit
> that is already dead: free it. */
> kfree(ppp);
>- else
>+ local_bh_enable(); /* Must re-enable bh twice */
>+ local_bh_enable();
>+ }else
> ppp_unlock(ppp);
> err = 0;
> }
Well, this twice-enabling looks somewhat strange, but anyhow, that's probably
just a preliminary work around for a ppp_genric problem you'e encontered.
I have not further analyzed this, but maybe a possible solution would be
to use an atomic_t usage counter for each ppp_channel which is
incremented whenever a channel or a device is attached and the kfree
triggered by an atomic_decrement_and_test() ...
>diff -r -N -u linux.orig/drivers/net/pppoe.c linux/drivers/net/pppoe.c
>--- linux.orig/drivers/net/pppoe.c Wed Dec 31 19:00:00 1969
>+++ linux/drivers/net/pppoe.c Sun Apr 16 20:24:51 2000
+/************************************************************************
+ * Receive a PPPoE Session frame.
+ ***********************************************************************/
+static int pppoe_rcv(struct sk_buff *skb,
+ if ( sk->state & PPPOX_BOUND ) {
+ skb_pull(skb, sizeof(struct pppoe_hdr));
+
+ ppp_input(&po->chan, skb);
+
+ }else{
+ sock_queue_rcv_skb(sk, skb);
Be aware of a possible memory leak if queueing failed here ...
+ }
+ return 1;
>+
>+/***********************************************************************
>+ * Initialize a new struct sock.
>+ **********************************************************************/
>+static int pppoe_create(struct socket *sock)
>+{
>+ sk->type = SOCK_STREAM;
SOCK_STREAM is probably a bad choice here because it does not fulfill
POSIX sematics of a reliable byte stream. I think SOCK_DGRAM would be
more appropriate here.
>+
>+ /* Delete the protinfo when it is time to do so. */
>+ sk->protinfo.destruct_hook = sk->protinfo.pppox;
This should be obsolete because sk->protinfo is a union and destruct_hook
is just a synonym for pppox.
>+int pppoe_release(struct socket *sock)
>+{
>+
>+ skb_queue_purge(&sk->receive_queue);
>+
>+ sock_put(sk);
There seems to be no handler for defered socket destroy. This could cause
problems, e.g. if tx skb's are still in the lower layer device queues
while socket is destroyed, and those skb's still hold a referece to that
socket. This will currently not be a problem with the ppp tx frames because
you never account them to the socket. But it might hit you if you queue
frames from user space and than suddenly close the socket. Maybe this explains
the kernel crashes that you have reported with earlier versions of the driver?
>+/************************************************************************
>+ *
>+ * xmit function called by generic PPP driver
>+ * sends PPP frame over PPPoE socket
>+ *
>+ ***********************************************************************/
>+int pppoe_xmit(struct ppp_channel *chan, struct sk_buff *skb)
>+{
>+
>+ dev_queue_xmit(skb);
>+
>+ /* Ready to xmit next packet --- or should the dev queue be checked
>+ * to make sure it isn't full?
>+ */
>+
>+ /* New ppp_generic code really doesn't like this...
>+ ppp_output_wakeup(chan);
>+ */
Just wondering: you completely bypass socket (wmem accounting) based flow
control here (you never set skb ownership for pppoe_xmit sk_buffs).
Subsequently, you cannot use this to trigger ppp_generic flow
control and your ppp channel is always willing to accept frames. Thus,
I think if you send to fast, packets will be lost. tcp will probably
realize this and adjust transfer speed, but how do other protocols behave?
BTW: in 2.3.x, dev_queue_xmit() returns a meaningful int indicating whether
queueing was successfull. I guess you could easily use this for triggering
ppp_generic flow control - no need for checking the dev queue explicitly.
Maybe this is sufficient for flow control and you really can (as you
already do) avoid socket wmem accounting based flow control.
(but then the harder task will be to wake up the channel again)
Henner
|