Possible IRDA SKB leaks

Possible IRDA SKB leaks

[David S. Miller]

Hello Jean, I think I've found some SKB handling bugs in
the IRDA stack.

I was verifying all the paths that use sock_queue_rcv_skb().
If any non-zero value is returned from this function the caller
must either free the SKB or queue the packet some place else.

Here is one example where IRDA appears to do the wrong thing.
In irttp_udata_indication(), we pass the packet down into the
next layer via self->notify.udata_indication().

One example implementation of this is af_irda.c:irda_data_indication().
This calls sock_queue_rcv_skb() and returns any error to the caller.

Our caller in this case, irttp_udata_indication(), for some reason
treats -ENOMEM specially.  This is wrong, there are many other errors
that sock_queue_rcv_skb() can return, for example -EPERM from socket
filtering.  All such error cases need to cause the SKB to be freed
or similar, it should not be done only for an error of -ENOMEM.

I have not done an exhaustive audit of this problem in the IRDA stack.
But I do suspect there are other places doing something similar.

If someone could finish the audit and submit a patch to fix this I'd
really appreciate it.  Thanks a lot.

Re: Possible IRDA SKB leaks

[Jean Tourrilhes]

On Mon, Nov 24, 2003 at 07:30:42PM -0800, David S. Miller wrote:
> 
> Hello Jean, I think I've found some SKB handling bugs in
> the IRDA stack.
> 
> I was verifying all the paths that use sock_queue_rcv_skb().
> If any non-zero value is returned from this function the caller
> must either free the SKB or queue the packet some place else.
> 
> Here is one example where IRDA appears to do the wrong thing.
> In irttp_udata_indication(), we pass the packet down into the
> next layer via self->notify.udata_indication().
> 
> One example implementation of this is af_irda.c:irda_data_indication().
> This calls sock_queue_rcv_skb() and returns any error to the caller.
> 
> Our caller in this case, irttp_udata_indication(), for some reason
> treats -ENOMEM specially.  This is wrong, there are many other errors
> that sock_queue_rcv_skb() can return, for example -EPERM from socket
> filtering.  All such error cases need to cause the SKB to be freed
> or similar, it should not be done only for an error of -ENOMEM.
> 
> I have not done an exhaustive audit of this problem in the IRDA stack.
> But I do suspect there are other places doing something similar.
> 
> If someone could finish the audit and submit a patch to fix this I'd
> really appreciate it.  Thanks a lot.

	Ok, I got it. The module af_irda is only ever called by
irttp.c, so that is going to be doable. I'll try to find time before
Thanksgiving, but it's short (meetings and co.).
	Thanks.

	Jean

Re: Possible IRDA SKB leaks

[Jean Tourrilhes]

On Mon, Nov 24, 2003 at 07:30:42PM -0800, David S. Miller wrote:
> 
> Hello Jean, I think I've found some SKB handling bugs in
> the IRDA stack.
> 
> I was verifying all the paths that use sock_queue_rcv_skb().
> If any non-zero value is returned from this function the caller
> must either free the SKB or queue the packet some place else.
> 
> Here is one example where IRDA appears to do the wrong thing.
> In irttp_udata_indication(), we pass the packet down into the
> next layer via self->notify.udata_indication().
> 
> One example implementation of this is af_irda.c:irda_data_indication().
> This calls sock_queue_rcv_skb() and returns any error to the caller.
> 
> Our caller in this case, irttp_udata_indication(), for some reason
> treats -ENOMEM specially.  This is wrong, there are many other errors
> that sock_queue_rcv_skb() can return, for example -EPERM from socket
> filtering.  All such error cases need to cause the SKB to be freed
> or similar, it should not be done only for an error of -ENOMEM.

	I would need to ask Dag to know the real reason. This code was
unchanged since 2.2.X days. What was true for 2.2.X is no longer true.

> I have not done an exhaustive audit of this problem in the IRDA stack.
> But I do suspect there are other places doing something similar.
> 
> If someone could finish the audit and submit a patch to fix this I'd
> really appreciate it.  Thanks a lot.

	I did the audit, and there was not many places to fix. Tested
on 2.6.0-test9 (both data and udata path). Patch is trivial and
attached below.
	Thanks for the heads up !

	Have fun...

	Jean

P.S. : I've got other important IrDA patches, but a freeze is a freeze ;-)

---------------------------------------

diff -u -p linux/net/irda/irttp.d6.c linux/net/irda/irttp.c
--- linux/net/irda/irttp.d6.c	Tue Nov 25 18:12:19 2003
+++ linux/net/irda/irttp.c	Tue Nov 25 18:17:24 2003
@@ -859,10 +859,10 @@ static int irttp_udata_indication(void *
 		err = self->notify.udata_indication(self->notify.instance,
 						    self,skb);
 		/* Same comment as in irttp_do_data_indication() */
-		if (err != -ENOMEM) 
+		if (!err) 
 			return 0;
 	}
-	/* Either no handler, or -ENOMEM */
+	/* Either no handler, or handler returns an error */
 	dev_kfree_skb(skb);
 
 	return 0;
@@ -1620,7 +1620,7 @@ void irttp_do_data_indication(struct tsa
 	 * be difficult, so it can instead just refuse to eat it and just
 	 * give an error back
 	 */
-	if (err == -ENOMEM) {
+	if (err) {
 		IRDA_DEBUG(0, "%s() requeueing skb!\n", __FUNCTION__);
 
 		/* Make sure we take a break */

Re: Possible IRDA SKB leaks

[David S. Miller]

On Tue, 25 Nov 2003 18:33:58 -0800
Jean Tourrilhes <jt@bougret.hpl.hp.com> wrote:

> 	I did the audit, and there was not many places to fix. Tested
> on 2.6.0-test9 (both data and udata path). Patch is trivial and
> attached below.
> 	Thanks for the heads up !

Thanks a lot for following through on this Jean.

Patch applied.