* [patch 1/5] binfmt: fix uaccess handling
2006-10-26 13:00 [patch 0/5] various user space access fixes Heiko Carstens
@ 2006-10-26 13:01 ` Heiko Carstens
2006-10-27 4:37 ` Andrew Morton
2006-10-26 13:02 ` [patch 2/5] compat: " Heiko Carstens
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Heiko Carstens @ 2006-10-26 13:01 UTC (permalink / raw)
To: Andrew Morton, linux-kernel
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
fs/binfmt_elf.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
Index: linux-2.6/fs/binfmt_elf.c
===================================================================
--- linux-2.6.orig/fs/binfmt_elf.c 2006-10-26 14:40:58.000000000 +0200
+++ linux-2.6/fs/binfmt_elf.c 2006-10-26 14:41:59.000000000 +0200
@@ -243,8 +243,9 @@
if (interp_aout) {
argv = sp + 2;
envp = argv + argc + 1;
- __put_user((elf_addr_t)(unsigned long)argv, sp++);
- __put_user((elf_addr_t)(unsigned long)envp, sp++);
+ if (__put_user((elf_addr_t)(unsigned long)argv, sp++) ||
+ __put_user((elf_addr_t)(unsigned long)envp, sp++))
+ return -EFAULT;
} else {
argv = sp;
envp = argv + argc + 1;
@@ -254,7 +255,8 @@
p = current->mm->arg_end = current->mm->arg_start;
while (argc-- > 0) {
size_t len;
- __put_user((elf_addr_t)p, argv++);
+ if (__put_user((elf_addr_t)p, argv++))
+ return -EFAULT;
len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
return 0;
@@ -265,7 +267,8 @@
current->mm->arg_end = current->mm->env_start = p;
while (envc-- > 0) {
size_t len;
- __put_user((elf_addr_t)p, envp++);
+ if (__put_user((elf_addr_t)p, envp++))
+ return -EFAULT;
len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
return 0;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 1/5] binfmt: fix uaccess handling
2006-10-26 13:01 ` [patch 1/5] binfmt: fix uaccess handling Heiko Carstens
@ 2006-10-27 4:37 ` Andrew Morton
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2006-10-27 4:37 UTC (permalink / raw)
To: Heiko Carstens; +Cc: linux-kernel
On Thu, 26 Oct 2006 15:01:46 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> @@ -254,7 +255,8 @@
> p = current->mm->arg_end = current->mm->arg_start;
> while (argc-- > 0) {
> size_t len;
> - __put_user((elf_addr_t)p, argv++);
> + if (__put_user((elf_addr_t)p, argv++))
> + return -EFAULT;
> len = strnlen_user((void __user *)p, PAGE_SIZE*MAX_ARG_PAGES);
> if (!len || len > PAGE_SIZE*MAX_ARG_PAGES)
> return 0;
We return EFAULT, but if strnlen_user() gets a fault, we return zero. But
then, the return value of create_elf_tables() gets cheerfully ignored
anyway. So I assume we start up some new program with a
partially-constructed environment and it then mysteriously malfunctions.
Nice, eh?
^ permalink raw reply [flat|nested] 11+ messages in thread
* [patch 2/5] compat: fix uaccess handling
2006-10-26 13:00 [patch 0/5] various user space access fixes Heiko Carstens
2006-10-26 13:01 ` [patch 1/5] binfmt: fix uaccess handling Heiko Carstens
@ 2006-10-26 13:02 ` Heiko Carstens
2006-10-26 13:03 ` [patch 3/5] net: " Heiko Carstens
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Heiko Carstens @ 2006-10-26 13:02 UTC (permalink / raw)
To: Andrew Morton, linux-kernel
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
fs/compat.c | 35 +++++++++++++++++++----------------
fs/compat_ioctl.c | 33 ++++++++++++++++++++-------------
2 files changed, 39 insertions(+), 29 deletions(-)
Index: linux-2.6/fs/compat.c
===================================================================
--- linux-2.6.orig/fs/compat.c 2006-10-26 14:40:56.000000000 +0200
+++ linux-2.6/fs/compat.c 2006-10-26 14:42:09.000000000 +0200
@@ -1142,7 +1142,9 @@
lastdirent = buf.previous;
if (lastdirent) {
typeof(lastdirent->d_off) d_off = file->f_pos;
- __put_user_unaligned(d_off, &lastdirent->d_off);
+ error = -EFAULT;
+ if (__put_user_unaligned(d_off, &lastdirent->d_off))
+ goto out_putf;
error = count - buf.count;
}
@@ -1609,14 +1611,14 @@
nr &= ~1UL;
while (nr) {
unsigned long h, l;
- __get_user(l, ufdset);
- __get_user(h, ufdset+1);
+ if (__get_user(l, ufdset) || __get_user(h, ufdset+1))
+ return -EFAULT;
ufdset += 2;
*fdset++ = h << 32 | l;
nr -= 2;
}
- if (odd)
- __get_user(*fdset, ufdset);
+ if (odd && __get_user(*fdset, ufdset))
+ return -EFAULT;
} else {
/* Tricky, must clear full unsigned long in the
* kernel fdset at the end, this makes sure that
@@ -1628,14 +1630,14 @@
}
static
-void compat_set_fd_set(unsigned long nr, compat_ulong_t __user *ufdset,
- unsigned long *fdset)
+int compat_set_fd_set(unsigned long nr, compat_ulong_t __user *ufdset,
+ unsigned long *fdset)
{
unsigned long odd;
nr = ROUND_UP(nr, __COMPAT_NFDBITS);
if (!ufdset)
- return;
+ return 0;
odd = nr & 1UL;
nr &= ~1UL;
@@ -1643,13 +1645,14 @@
unsigned long h, l;
l = *fdset++;
h = l >> 32;
- __put_user(l, ufdset);
- __put_user(h, ufdset+1);
+ if (__put_user(l, ufdset) || __put_user(h, ufdset+1))
+ return -EFAULT;
ufdset += 2;
nr -= 2;
}
- if (odd)
- __put_user(*fdset, ufdset);
+ if (odd && __put_user(*fdset, ufdset))
+ return -EFAULT;
+ return 0;
}
@@ -1724,10 +1727,10 @@
ret = 0;
}
- compat_set_fd_set(n, inp, fds.res_in);
- compat_set_fd_set(n, outp, fds.res_out);
- compat_set_fd_set(n, exp, fds.res_ex);
-
+ if (compat_set_fd_set(n, inp, fds.res_in) ||
+ compat_set_fd_set(n, outp, fds.res_out) ||
+ compat_set_fd_set(n, exp, fds.res_ex))
+ ret = -EFAULT;
out:
kfree(bits);
out_nofds:
Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c 2006-10-26 14:40:56.000000000 +0200
+++ linux-2.6/fs/compat_ioctl.c 2006-10-26 14:42:09.000000000 +0200
@@ -211,8 +211,10 @@
up_native =
compat_alloc_user_space(sizeof(struct video_still_picture));
- put_user(compat_ptr(fp), &up_native->iFrame);
- put_user(size, &up_native->size);
+ err = put_user(compat_ptr(fp), &up_native->iFrame);
+ err |= put_user(size, &up_native->size);
+ if (err)
+ return -EFAULT;
err = sys_ioctl(fd, cmd, (unsigned long) up_native);
@@ -236,8 +238,10 @@
err |= get_user(length, &up->length);
up_native = compat_alloc_user_space(sizeof(struct video_spu_palette));
- put_user(compat_ptr(palp), &up_native->palette);
- put_user(length, &up_native->length);
+ err = put_user(compat_ptr(palp), &up_native->palette);
+ err |= put_user(length, &up_native->length);
+ if (err)
+ return -EFAULT;
err = sys_ioctl(fd, cmd, (unsigned long) up_native);
@@ -2043,16 +2047,19 @@
struct serial_struct ss;
mm_segment_t oldseg = get_fs();
__u32 udata;
+ unsigned int base;
if (cmd == TIOCSSERIAL) {
if (!access_ok(VERIFY_READ, ss32, sizeof(SS32)))
return -EFAULT;
if (__copy_from_user(&ss, ss32, offsetof(SS32, iomem_base)))
return -EFAULT;
- __get_user(udata, &ss32->iomem_base);
+ if (__get_user(udata, &ss32->iomem_base))
+ return -EFAULT;
ss.iomem_base = compat_ptr(udata);
- __get_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift);
- __get_user(ss.port_high, &ss32->port_high);
+ if (__get_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift) ||
+ __get_user(ss.port_high, &ss32->port_high))
+ return -EFAULT;
ss.iomap_base = 0UL;
}
set_fs(KERNEL_DS);
@@ -2063,12 +2070,12 @@
return -EFAULT;
if (__copy_to_user(ss32,&ss,offsetof(SS32,iomem_base)))
return -EFAULT;
- __put_user((unsigned long)ss.iomem_base >> 32 ?
- 0xffffffff : (unsigned)(unsigned long)ss.iomem_base,
- &ss32->iomem_base);
- __put_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift);
- __put_user(ss.port_high, &ss32->port_high);
-
+ base = (unsigned long)ss.iomem_base >> 32 ?
+ 0xffffffff : (unsigned)(unsigned long)ss.iomem_base;
+ if (__put_user(base, &ss32->iomem_base) ||
+ __put_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift) ||
+ __put_user(ss.port_high, &ss32->port_high))
+ return -EFAULT;
}
return err;
}
^ permalink raw reply [flat|nested] 11+ messages in thread* [patch 3/5] net: fix uaccess handling
2006-10-26 13:00 [patch 0/5] various user space access fixes Heiko Carstens
2006-10-26 13:01 ` [patch 1/5] binfmt: fix uaccess handling Heiko Carstens
2006-10-26 13:02 ` [patch 2/5] compat: " Heiko Carstens
@ 2006-10-26 13:03 ` Heiko Carstens
2006-10-30 23:06 ` David Miller
2006-10-26 13:04 ` [patch 4/5] profile: " Heiko Carstens
2006-10-26 13:04 ` [patch 5/5] scsi: " Heiko Carstens
4 siblings, 1 reply; 11+ messages in thread
From: Heiko Carstens @ 2006-10-26 13:03 UTC (permalink / raw)
To: Andrew Morton, linux-kernel; +Cc: netdev, David S. Miller, Jeff Garzik
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
net/ipv4/raw.c | 17 +++++++++++------
net/ipv6/raw.c | 17 +++++++++++------
net/netlink/af_netlink.c | 5 +++--
3 files changed, 25 insertions(+), 14 deletions(-)
Index: linux-2.6/net/ipv4/raw.c
===================================================================
--- linux-2.6.orig/net/ipv4/raw.c 2006-10-26 14:40:56.000000000 +0200
+++ linux-2.6/net/ipv4/raw.c 2006-10-26 14:42:12.000000000 +0200
@@ -329,7 +329,7 @@
return err;
}
-static void raw_probe_proto_opt(struct flowi *fl, struct msghdr *msg)
+static int raw_probe_proto_opt(struct flowi *fl, struct msghdr *msg)
{
struct iovec *iov;
u8 __user *type = NULL;
@@ -338,7 +338,7 @@
unsigned int i;
if (!msg->msg_iov)
- return;
+ return 0;
for (i = 0; i < msg->msg_iovlen; i++) {
iov = &msg->msg_iov[i];
@@ -360,8 +360,9 @@
code = iov->iov_base;
if (type && code) {
- get_user(fl->fl_icmp_type, type);
- get_user(fl->fl_icmp_code, code);
+ if (get_user(fl->fl_icmp_type, type) ||
+ get_user(fl->fl_icmp_code, code))
+ return -EFAULT;
probed = 1;
}
break;
@@ -372,6 +373,7 @@
if (probed)
break;
}
+ return 0;
}
static int raw_sendmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
@@ -480,8 +482,11 @@
.proto = inet->hdrincl ? IPPROTO_RAW :
sk->sk_protocol,
};
- if (!inet->hdrincl)
- raw_probe_proto_opt(&fl, msg);
+ if (!inet->hdrincl) {
+ err = raw_probe_proto_opt(&fl, msg);
+ if (err)
+ goto done;
+ }
security_sk_classify_flow(sk, &fl);
err = ip_route_output_flow(&rt, &fl, sk, !(msg->msg_flags&MSG_DONTWAIT));
Index: linux-2.6/net/ipv6/raw.c
===================================================================
--- linux-2.6.orig/net/ipv6/raw.c 2006-10-26 14:40:56.000000000 +0200
+++ linux-2.6/net/ipv6/raw.c 2006-10-26 14:42:12.000000000 +0200
@@ -604,7 +604,7 @@
return err;
}
-static void rawv6_probe_proto_opt(struct flowi *fl, struct msghdr *msg)
+static int rawv6_probe_proto_opt(struct flowi *fl, struct msghdr *msg)
{
struct iovec *iov;
u8 __user *type = NULL;
@@ -616,7 +616,7 @@
int i;
if (!msg->msg_iov)
- return;
+ return 0;
for (i = 0; i < msg->msg_iovlen; i++) {
iov = &msg->msg_iov[i];
@@ -638,8 +638,9 @@
code = iov->iov_base;
if (type && code) {
- get_user(fl->fl_icmp_type, type);
- get_user(fl->fl_icmp_code, code);
+ if (get_user(fl->fl_icmp_type, type) ||
+ get_user(fl->fl_icmp_code, code))
+ return -EFAULT;
probed = 1;
}
break;
@@ -650,7 +651,8 @@
/* check if type field is readable or not. */
if (iov->iov_len > 2 - len) {
u8 __user *p = iov->iov_base;
- get_user(fl->fl_mh_type, &p[2 - len]);
+ if (get_user(fl->fl_mh_type, &p[2 - len]))
+ return -EFAULT;
probed = 1;
} else
len += iov->iov_len;
@@ -664,6 +666,7 @@
if (probed)
break;
}
+ return 0;
}
static int rawv6_sendmsg(struct kiocb *iocb, struct sock *sk,
@@ -787,7 +790,9 @@
opt = ipv6_fixup_options(&opt_space, opt);
fl.proto = proto;
- rawv6_probe_proto_opt(&fl, msg);
+ err = rawv6_probe_proto_opt(&fl, msg);
+ if (err)
+ goto out;
ipv6_addr_copy(&fl.fl6_dst, daddr);
if (ipv6_addr_any(&fl.fl6_src) && !ipv6_addr_any(&np->saddr))
Index: linux-2.6/net/netlink/af_netlink.c
===================================================================
--- linux-2.6.orig/net/netlink/af_netlink.c 2006-10-26 14:40:56.000000000 +0200
+++ linux-2.6/net/netlink/af_netlink.c 2006-10-26 14:42:12.000000000 +0200
@@ -1075,8 +1075,9 @@
return -EINVAL;
len = sizeof(int);
val = nlk->flags & NETLINK_RECV_PKTINFO ? 1 : 0;
- put_user(len, optlen);
- put_user(val, optval);
+ if (put_user(len, optlen) ||
+ put_user(val, optval))
+ return -EFAULT;
err = 0;
break;
default:
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 3/5] net: fix uaccess handling
2006-10-26 13:03 ` [patch 3/5] net: " Heiko Carstens
@ 2006-10-30 23:06 ` David Miller
0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2006-10-30 23:06 UTC (permalink / raw)
To: heiko.carstens; +Cc: akpm, linux-kernel, netdev, jgarzik
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Thu, 26 Oct 2006 15:03:17 +0200
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
> net/ipv4/raw.c | 17 +++++++++++------
> net/ipv6/raw.c | 17 +++++++++++------
> net/netlink/af_netlink.c | 5 +++--
> 3 files changed, 25 insertions(+), 14 deletions(-)
Patch applied, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [patch 4/5] profile: fix uaccess handling
2006-10-26 13:00 [patch 0/5] various user space access fixes Heiko Carstens
` (2 preceding siblings ...)
2006-10-26 13:03 ` [patch 3/5] net: " Heiko Carstens
@ 2006-10-26 13:04 ` Heiko Carstens
2006-10-26 13:04 ` [patch 5/5] scsi: " Heiko Carstens
4 siblings, 0 replies; 11+ messages in thread
From: Heiko Carstens @ 2006-10-26 13:04 UTC (permalink / raw)
To: Andrew Morton, linux-kernel
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
kernel/profile.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6/kernel/profile.c
===================================================================
--- linux-2.6.orig/kernel/profile.c 2006-10-26 14:40:57.000000000 +0200
+++ linux-2.6/kernel/profile.c 2006-10-26 14:42:07.000000000 +0200
@@ -442,7 +442,8 @@
read = 0;
while (p < sizeof(unsigned int) && count > 0) {
- put_user(*((char *)(&sample_step)+p),buf);
+ if (put_user(*((char *)(&sample_step)+p),buf))
+ return -EFAULT;
buf++; p++; count--; read++;
}
pnt = (char *)prof_buffer + p - sizeof(atomic_t);
^ permalink raw reply [flat|nested] 11+ messages in thread* [patch 5/5] scsi: fix uaccess handling
2006-10-26 13:00 [patch 0/5] various user space access fixes Heiko Carstens
` (3 preceding siblings ...)
2006-10-26 13:04 ` [patch 4/5] profile: " Heiko Carstens
@ 2006-10-26 13:04 ` Heiko Carstens
2006-10-28 11:31 ` Christoph Hellwig
4 siblings, 1 reply; 11+ messages in thread
From: Heiko Carstens @ 2006-10-26 13:04 UTC (permalink / raw)
To: Andrew Morton, linux-kernel; +Cc: linux-scsi, James Bottomley
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
drivers/scsi/scsi_ioctl.c | 17 +++++++++-------
drivers/scsi/sg.c | 47 ++++++++++++++++++++++++----------------------
2 files changed, 35 insertions(+), 29 deletions(-)
Index: linux-2.6/drivers/scsi/scsi_ioctl.c
===================================================================
--- linux-2.6.orig/drivers/scsi/scsi_ioctl.c 2006-10-26 14:40:55.000000000 +0200
+++ linux-2.6/drivers/scsi/scsi_ioctl.c 2006-10-26 14:42:14.000000000 +0200
@@ -217,13 +217,16 @@
if (!access_ok(VERIFY_WRITE, arg, sizeof(struct scsi_idlun)))
return -EFAULT;
- __put_user((sdev->id & 0xff)
- + ((sdev->lun & 0xff) << 8)
- + ((sdev->channel & 0xff) << 16)
- + ((sdev->host->host_no & 0xff) << 24),
- &((struct scsi_idlun __user *)arg)->dev_id);
- __put_user(sdev->host->unique_id,
- &((struct scsi_idlun __user *)arg)->host_unique_id);
+ if (__put_user((sdev->id & 0xff)
+ + ((sdev->lun & 0xff) << 8)
+ + ((sdev->channel & 0xff) << 16)
+ + ((sdev->host->host_no & 0xff) << 24),
+ &((struct scsi_idlun __user *)arg)->dev_id))
+ return -EFAULT;
+
+ if(__put_user(sdev->host->unique_id,
+ &((struct scsi_idlun __user *)arg)->host_unique_id))
+ return -EFAULT;
return 0;
case SCSI_IOCTL_GET_BUS_NUMBER:
return put_user(sdev->host->host_no, (int __user *)arg);
Index: linux-2.6/drivers/scsi/sg.c
===================================================================
--- linux-2.6.orig/drivers/scsi/sg.c 2006-10-26 14:40:55.000000000 +0200
+++ linux-2.6/drivers/scsi/sg.c 2006-10-26 14:42:14.000000000 +0200
@@ -556,7 +556,8 @@
return -EDOM;
}
buf += SZ_SG_HEADER;
- __get_user(opcode, buf);
+ if (__get_user(opcode, buf))
+ return -EFAULT;
if (sfp->next_cmd_len > 0) {
if (sfp->next_cmd_len > MAX_COMMAND_SIZE) {
SCSI_LOG_TIMEOUT(1, printk("sg_write: command length too long\n"));
@@ -779,6 +780,7 @@
Sg_fd *sfp;
Sg_request *srp;
unsigned long iflags;
+ sg_scsi_id_t __user *sg_idp;
if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
return -ENXIO;
@@ -860,26 +862,25 @@
case SG_GET_SCSI_ID:
if (!access_ok(VERIFY_WRITE, p, sizeof (sg_scsi_id_t)))
return -EFAULT;
- else {
- sg_scsi_id_t __user *sg_idp = p;
+ sg_idp = p;
- if (sdp->detached)
- return -ENODEV;
- __put_user((int) sdp->device->host->host_no,
- &sg_idp->host_no);
- __put_user((int) sdp->device->channel,
- &sg_idp->channel);
- __put_user((int) sdp->device->id, &sg_idp->scsi_id);
- __put_user((int) sdp->device->lun, &sg_idp->lun);
- __put_user((int) sdp->device->type, &sg_idp->scsi_type);
- __put_user((short) sdp->device->host->cmd_per_lun,
- &sg_idp->h_cmd_per_lun);
- __put_user((short) sdp->device->queue_depth,
- &sg_idp->d_queue_depth);
- __put_user(0, &sg_idp->unused[0]);
- __put_user(0, &sg_idp->unused[1]);
- return 0;
- }
+ if (sdp->detached)
+ return -ENODEV;
+ if (__put_user((int) sdp->device->host->host_no,
+ &sg_idp->host_no) ||
+ __put_user((int) sdp->device->channel,
+ &sg_idp->channel) ||
+ __put_user((int) sdp->device->id, &sg_idp->scsi_id) ||
+ __put_user((int) sdp->device->lun, &sg_idp->lun) ||
+ __put_user((int) sdp->device->type, &sg_idp->scsi_type) ||
+ __put_user((short) sdp->device->host->cmd_per_lun,
+ &sg_idp->h_cmd_per_lun) ||
+ __put_user((short) sdp->device->queue_depth,
+ &sg_idp->d_queue_depth) ||
+ __put_user(0, &sg_idp->unused[0]) ||
+ __put_user(0, &sg_idp->unused[1]))
+ return -EFAULT;
+ return 0;
case SG_SET_FORCE_PACK_ID:
result = get_user(val, ip);
if (result)
@@ -894,12 +895,14 @@
if ((1 == srp->done) && (!srp->sg_io_owned)) {
read_unlock_irqrestore(&sfp->rq_list_lock,
iflags);
- __put_user(srp->header.pack_id, ip);
+ if (__put_user(srp->header.pack_id, ip))
+ return -EFAULT;
return 0;
}
}
read_unlock_irqrestore(&sfp->rq_list_lock, iflags);
- __put_user(-1, ip);
+ if (__put_user(-1, ip))
+ return -EFAULT;
return 0;
case SG_GET_NUM_WAITING:
read_lock_irqsave(&sfp->rq_list_lock, iflags);
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 5/5] scsi: fix uaccess handling
2006-10-26 13:04 ` [patch 5/5] scsi: " Heiko Carstens
@ 2006-10-28 11:31 ` Christoph Hellwig
2006-10-29 21:39 ` Heiko Carstens
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2006-10-28 11:31 UTC (permalink / raw)
To: Heiko Carstens; +Cc: Andrew Morton, linux-kernel, linux-scsi, James Bottomley
On Thu, Oct 26, 2006 at 03:04:52PM +0200, Heiko Carstens wrote:
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
> drivers/scsi/scsi_ioctl.c | 17 +++++++++-------
> drivers/scsi/sg.c | 47 ++++++++++++++++++++++++----------------------
> 2 files changed, 35 insertions(+), 29 deletions(-)
>
> Index: linux-2.6/drivers/scsi/scsi_ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/scsi_ioctl.c 2006-10-26 14:40:55.000000000 +0200
> +++ linux-2.6/drivers/scsi/scsi_ioctl.c 2006-10-26 14:42:14.000000000 +0200
> @@ -217,13 +217,16 @@
> if (!access_ok(VERIFY_WRITE, arg, sizeof(struct scsi_idlun)))
> return -EFAULT;
>
> - __put_user((sdev->id & 0xff)
> - + ((sdev->lun & 0xff) << 8)
> - + ((sdev->channel & 0xff) << 16)
> - + ((sdev->host->host_no & 0xff) << 24),
> - &((struct scsi_idlun __user *)arg)->dev_id);
> - __put_user(sdev->host->unique_id,
> - &((struct scsi_idlun __user *)arg)->host_unique_id);
> + if (__put_user((sdev->id & 0xff)
> + + ((sdev->lun & 0xff) << 8)
> + + ((sdev->channel & 0xff) << 16)
> + + ((sdev->host->host_no & 0xff) << 24),
> + &((struct scsi_idlun __user *)arg)->dev_id))
> + return -EFAULT;
> +
While not your fault I'd suggest to fix the __put_user abuse at the same
time, as in the untested patch below for scsi_ioctl.c:
Index: linux-2.6/drivers/scsi/scsi_ioctl.c
===================================================================
--- linux-2.6.orig/drivers/scsi/scsi_ioctl.c 2006-10-28 13:24:18.000000000 +0200
+++ linux-2.6/drivers/scsi/scsi_ioctl.c 2006-10-28 13:30:17.000000000 +0200
@@ -173,6 +173,21 @@
return copy_to_user(arg, dev->bus_id, sizeof(dev->bus_id))? -EFAULT: 0;
}
+static int scsi_get_idlun(struct scsi_device *sdev,
+ struct scsi_idlun __user *arg)
+{
+ struct scsi_idlun karg = {
+ .dev_id = (sdev->id & 0xff) +
+ ((sdev->lun & 0xff) << 8) +
+ ((sdev->channel & 0xff) << 16) +
+ ((sdev->host->host_no & 0xff) << 24),
+ .host_unique_id = sdev->host->unique_id
+ };
+
+ if (copy_to_user(arg, &karg, sizeof(struct scsi_idlun)))
+ return -EFAULT;
+ return 0;
+}
/*
* the scsi_ioctl() function differs from most ioctls in that it does
@@ -214,17 +229,7 @@
switch (cmd) {
case SCSI_IOCTL_GET_IDLUN:
- if (!access_ok(VERIFY_WRITE, arg, sizeof(struct scsi_idlun)))
- return -EFAULT;
-
- __put_user((sdev->id & 0xff)
- + ((sdev->lun & 0xff) << 8)
- + ((sdev->channel & 0xff) << 16)
- + ((sdev->host->host_no & 0xff) << 24),
- &((struct scsi_idlun __user *)arg)->dev_id);
- __put_user(sdev->host->unique_id,
- &((struct scsi_idlun __user *)arg)->host_unique_id);
- return 0;
+ return scsi_get_idlun(sdev, arg);
case SCSI_IOCTL_GET_BUS_NUMBER:
return put_user(sdev->host->host_no, (int __user *)arg);
case SCSI_IOCTL_PROBE_HOST:
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 5/5] scsi: fix uaccess handling
2006-10-28 11:31 ` Christoph Hellwig
@ 2006-10-29 21:39 ` Heiko Carstens
2006-12-15 17:36 ` James Bottomley
0 siblings, 1 reply; 11+ messages in thread
From: Heiko Carstens @ 2006-10-29 21:39 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, linux-kernel, linux-scsi, James Bottomley
> While not your fault I'd suggest to fix the __put_user abuse at the same
> time, as in the untested patch below for scsi_ioctl.c:
Makes sense. Even though the whole SCSI_IOCTL_GET_IDLUN ioctl interface
is pretty pointless.
It supports only up to 255 different ids and luns and might return the
same 'dev_id' for two different devices...
Any user space utility that depends on this interface would do the wrong
thing (whatever that would be).
> Index: linux-2.6/drivers/scsi/scsi_ioctl.c
> ===================================================================
> --- linux-2.6.orig/drivers/scsi/scsi_ioctl.c 2006-10-28 13:24:18.000000000 +0200
> +++ linux-2.6/drivers/scsi/scsi_ioctl.c 2006-10-28 13:30:17.000000000 +0200
> @@ -173,6 +173,21 @@
> return copy_to_user(arg, dev->bus_id, sizeof(dev->bus_id))? -EFAULT: 0;
> }
>
> +static int scsi_get_idlun(struct scsi_device *sdev,
> + struct scsi_idlun __user *arg)
> +{
> + struct scsi_idlun karg = {
> + .dev_id = (sdev->id & 0xff) +
> + ((sdev->lun & 0xff) << 8) +
> + ((sdev->channel & 0xff) << 16) +
> + ((sdev->host->host_no & 0xff) << 24),
> + .host_unique_id = sdev->host->unique_id
> + };
> +
> + if (copy_to_user(arg, &karg, sizeof(struct scsi_idlun)))
> + return -EFAULT;
> + return 0;
> +}
>
> /*
> * the scsi_ioctl() function differs from most ioctls in that it does
> @@ -214,17 +229,7 @@
>
> switch (cmd) {
> case SCSI_IOCTL_GET_IDLUN:
> - if (!access_ok(VERIFY_WRITE, arg, sizeof(struct scsi_idlun)))
> - return -EFAULT;
> -
> - __put_user((sdev->id & 0xff)
> - + ((sdev->lun & 0xff) << 8)
> - + ((sdev->channel & 0xff) << 16)
> - + ((sdev->host->host_no & 0xff) << 24),
> - &((struct scsi_idlun __user *)arg)->dev_id);
> - __put_user(sdev->host->unique_id,
> - &((struct scsi_idlun __user *)arg)->host_unique_id);
> - return 0;
> + return scsi_get_idlun(sdev, arg);
> case SCSI_IOCTL_GET_BUS_NUMBER:
> return put_user(sdev->host->host_no, (int __user *)arg);
> case SCSI_IOCTL_PROBE_HOST:
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [patch 5/5] scsi: fix uaccess handling
2006-10-29 21:39 ` Heiko Carstens
@ 2006-12-15 17:36 ` James Bottomley
0 siblings, 0 replies; 11+ messages in thread
From: James Bottomley @ 2006-12-15 17:36 UTC (permalink / raw)
To: Heiko Carstens; +Cc: Christoph Hellwig, Andrew Morton, linux-kernel, linux-scsi
On Sun, 2006-10-29 at 22:39 +0100, Heiko Carstens wrote:
> > While not your fault I'd suggest to fix the __put_user abuse at the same
> > time, as in the untested patch below for scsi_ioctl.c:
>
> Makes sense. Even though the whole SCSI_IOCTL_GET_IDLUN ioctl interface
> is pretty pointless.
> It supports only up to 255 different ids and luns and might return the
> same 'dev_id' for two different devices...
> Any user space utility that depends on this interface would do the wrong
> thing (whatever that would be).
So has anyone actually tested this?
James
^ permalink raw reply [flat|nested] 11+ messages in thread