Messages in this thread Patch in this message | | | Date | Thu, 20 Apr 2006 23:14:32 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH][resend] ISDN: unsafe interaction between isdn_write and isdn_writebuf_stub |
| |
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.
- 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/
| |