[PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub

[PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub

[Jesper Juhl]

(patch previously send on April 11'th - this is a resend)

isdn_write() and isdn_writebuf_stub() seem to have some unsafe interaction.

I was originally just looking to fix this warning:
  drivers/isdn/i4l/isdn_common.c:1956: warning: ignoring return value of `opy_from_user', declared with attribute warn_unused_result
And indeed, the return value is not checked, and I can't convince myself
that it's 100% certain that it can never fail.

While reading the code I also noticed that the while loop in isdn_write()
only tests for isdn_writebuf_stub() return value != count as termination
condition. This makes it impossible for isdn_writebuf_stub() to tell the 
caller why it failed so the caller can pass that info on.
It also looks unsafe that if isdn_writebuf_stub() fails to allocate an skb,
then it just returns 0 (zero) which is unlikely to cause the != count 
check in the caller to abort the loop, so it looks like it'll just enter 
the function once more and again fail to alloc an skb, repeat ad infinitum.

To fix these things I first made isdn_writebuf_stub() return -ENOMEM if it 
cannot allocate an skb and also return -EFAULT if the user copy fails.
 (this ofcourse also fixes the warning I was originally investigating)

Then I ditched the while loop in isdn_write() and replaced it with a 
hand-coded loop made up of a label and a goto, and inside this hand-made 
loop I then test if isdn_writebuf_stub() returns a value <=0 and if it does
then that value is used as the `retval' from isdn_write() and if not then 
it tests the !=count condition and otherwise behaves like the original 
while loop.


I hope my analysis of the situation and the resulting fix is correct; if 
not, then I'd appreciate feedback pointing out my error(s).

Unfortunately I have no hardware to properly test the patch, so it's 
compile tested only. So please read the patch carefully before applying it.


Tilman Schmidt gave me some feedback : 
"... my development machine has been running with your patch for a
while, with no apparent ill effects. Although this hardly qualifies as
exhaustive testing, it does seem do indicate that the patch is on the
right track."


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 drivers/isdn/i4l/isdn_common.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

--- linux-2.6.17-rc1-git4-orig/drivers/isdn/i4l/isdn_common.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc1-git4/drivers/isdn/i4l/isdn_common.c	2006-04-11 21:43:26.000000000 +0200
@@ -1177,9 +1177,14 @@ isdn_write(struct file *file, const char
 			goto out;
 		}
 		chidx = isdn_minor2chan(minor);
-		while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count)
+ loop:
+		retval = isdn_writebuf_stub(drvidx, chidx, buf, count);
+		if (retval < 0)
+			goto out;
+		if (retval != count) {
 			interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]);
-		retval = count;
+			goto loop;
+		}
 		goto out;
 	}
 	if (minor <= ISDN_MINOR_CTRLMAX) {
@@ -1951,9 +1956,10 @@ isdn_writebuf_stub(int drvidx, int chan,
 	struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
 
 	if (!skb)
-		return 0;
+		return -ENOMEM;
 	skb_reserve(skb, hl);
-	copy_from_user(skb_put(skb, len), buf, len);
+	if (!copy_from_user(skb_put(skb, len), buf, len))
+		return -EFAULT;
 	ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb);
 	if (ret <= 0)
 		dev_kfree_skb(skb);

Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> --- linux-2.6.17-rc1-git4-orig/drivers/isdn/i4l/isdn_common.c	2006-03-20 06:53:29.000000000 +0100
>  +++ linux-2.6.17-rc1-git4/drivers/isdn/i4l/isdn_common.c	2006-04-11 21:43:26.000000000 +0200
>  @@ -1177,9 +1177,14 @@ isdn_write(struct file *file, const char
>   			goto out;
>   		}
>   		chidx = isdn_minor2chan(minor);
>  -		while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count)
>  + loop:
>  +		retval = isdn_writebuf_stub(drvidx, chidx, buf, count);
>  +		if (retval < 0)
>  +			goto out;
>  +		if (retval != count) {
>   			interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]);
>  -		retval = count;
>  +			goto loop;
>  +		}
>   		goto out;
>   	}
>   	if (minor <= ISDN_MINOR_CTRLMAX) {
>  @@ -1951,9 +1956,10 @@ isdn_writebuf_stub(int drvidx, int chan,
>   	struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
>   
>   	if (!skb)
>  -		return 0;
>  +		return -ENOMEM;
>   	skb_reserve(skb, hl);
>  -	copy_from_user(skb_put(skb, len), buf, len);
>  +	if (!copy_from_user(skb_put(skb, len), buf, len))
>  +		return -EFAULT;
>   	ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb);
>   	if (ret <= 0)
>   		dev_kfree_skb(skb);

It's simpler to code it this way:



From: Jesper Juhl <jesper.juhl@gmail.com>

isdn_writebuf_stub() forgets to detect memory allocation and uaccess errors. 
And when that's fixed, if a error happens the caller will just keep on
looping.

So change the caller to detect the error, and to return it.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Cc: Karsten Keil <kkeil@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 drivers/isdn/i4l/isdn_common.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff -puN drivers/isdn/i4l/isdn_common.c~isdn-unsafe-interaction-between-isdn_write-and-isdn_writebuf_stub drivers/isdn/i4l/isdn_common.c
--- devel/drivers/isdn/i4l/isdn_common.c~isdn-unsafe-interaction-between-isdn_write-and-isdn_writebuf_stub	2006-04-20 23:02:53.000000000 -0700
+++ devel-akpm/drivers/isdn/i4l/isdn_common.c	2006-04-20 23:09:18.000000000 -0700
@@ -1177,9 +1177,12 @@ isdn_write(struct file *file, const char
 			goto out;
 		}
 		chidx = isdn_minor2chan(minor);
-		while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count)
+		for ( ; ; ) {
+			retval = isdn_writebuf_stub(drvidx, chidx, buf, count);
+			if (retval < 0 || retval == count)
+				break;
 			interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]);
-		retval = count;
+		}
 		goto out;
 	}
 	if (minor <= ISDN_MINOR_CTRLMAX) {
@@ -1951,9 +1954,10 @@ isdn_writebuf_stub(int drvidx, int chan,
 	struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);
 
 	if (!skb)
-		return 0;
+		return -ENOMEM;
 	skb_reserve(skb, hl);
-	copy_from_user(skb_put(skb, len), buf, len);
+	if (!copy_from_user(skb_put(skb, len), buf, len))
+		return -EFAULT;
 	ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb);
 	if (ret <= 0)
 		dev_kfree_skb(skb);
_






But the code still looks wrong.  If isdn_writebuf_stub() does a short write, we'll
just retry the entire write.  And if that returns the same short write, we'll
retry the write again, ad infinitum.

One would expect that if a short write happened, we either bale out with an
error or we advance partway through the buffer and write some more.

But I can't even spell ISDN.

Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 2776 bytes --]

On 21.04.2006 08:14, Andrew Morton wrote:
> It's simpler to code it this way:
[snip]
> But the code still looks wrong.  If isdn_writebuf_stub() does a short write, we'll
> just retry the entire write.  And if that returns the same short write, we'll
> retry the write again, ad infinitum.

You're right of course. But as i4l has never been able to handle that
case correctly, i4l drivers just don't do short writes, period. They
either return a negative error code, zero to indicate "not ready",
or the requested length. Every i4l driver author learns that very
quickly. (At least I did. ;-)

> One would expect that if a short write happened, we either bale out with an
> error or we advance partway through the buffer and write some more.

isdn_write() is the write method of i4l's character device. If a short
write would indeed occur, just passing it on to the caller should be
ok, too. So I'd propose the following, even simpler version:



From: Jesper Juhl <jesper.juhl@gmail.com>

isdn_writebuf_stub() forgets to detect memory allocation and uaccess errors.
And when that's fixed, if a error happens the caller will just keep on
looping.

So change the caller to detect the error, and to return it.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Cc: Karsten Keil <kkeil@suse.de>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Tilman Schmidt <tilman@imap.cc>

---

 drivers/isdn/i4l/isdn_common.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff -puN linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c
--- linux-2.6.17-rc2-orig/drivers/isdn/i4l/isdn_common.c	2006-03-20 06:53:29.000000000 +0100
+++ linux-2.6.17-rc2-work/drivers/isdn/i4l/isdn_common.c	2006-04-21 15:09:41.000000000 +0200
@@ -1177,9 +1177,8 @@ isdn_write(struct file *file, const char
 			goto out;
 		}
 		chidx = isdn_minor2chan(minor);
-		while (isdn_writebuf_stub(drvidx, chidx, buf, count) != count)
+		while ((retval = isdn_writebuf_stub(drvidx, chidx, buf, count)) == 0)
 			interruptible_sleep_on(&dev->drv[drvidx]->snd_waitq[chidx]);
-		retval = count;
 		goto out;
 	}
 	if (minor <= ISDN_MINOR_CTRLMAX) {
@@ -1951,9 +1950,10 @@ isdn_writebuf_stub(int drvidx, int chan,
 	struct sk_buff *skb = alloc_skb(hl + len, GFP_ATOMIC);

 	if (!skb)
-		return 0;
+		return -ENOMEM;
 	skb_reserve(skb, hl);
-	copy_from_user(skb_put(skb, len), buf, len);
+	if (!copy_from_user(skb_put(skb, len), buf, len))
+		return -EFAULT;
 	ret = dev->drv[drvidx]->interface->writebuf_skb(drvidx, chan, 1, skb);
 	if (ret <= 0)
 		dev_kfree_skb(skb);

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Reality is that which, when you stop believing in it, does not go
away. (Philip K. Dick)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]