Re: Possible IRDA SKB leaks

[Jean Tourrilhes <jt@bougret.hpl.hp.com>]

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 */