* [PATCH] megaraid_sas: fix for 32bit apps @ 2010-02-05 17:04 Tomas Henzl 2010-02-10 19:36 ` James Bottomley 0 siblings, 1 reply; 3+ messages in thread From: Tomas Henzl @ 2010-02-05 17:04 UTC (permalink / raw) To: linux-scsi; +Cc: Yang, Bo, James.Bottomley, hch, thenzl It looks like this patch - http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7b2519afa1abd1b9f63aa1e90879307842422dae has caused a problem for 32bit programs with 64bit os - http://bugzilla.kernel.org/show_bug.cgi?id=15001 megaraid: convert user space 32bit pointer to a 64 bit one when needed. Signed-off-by: Tomas Henzl <thenzl@redhat.com> diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c index 708ea31..7617b8e 100644 --- a/drivers/scsi/megaraid/megaraid_sas.c +++ b/drivers/scsi/megaraid/megaraid_sas.c @@ -3781,6 +3781,8 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) compat_alloc_user_space(sizeof(struct megasas_iocpacket)); int i; int error = 0; + compat_uptr_t ptr; + u8 *sense_ptr; if (clear_user(ioc, sizeof(*ioc))) return -EFAULT; @@ -3793,9 +3795,19 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32))) return -EFAULT; - for (i = 0; i < MAX_IOCTL_SGE; i++) { - compat_uptr_t ptr; + /* + * The sense_ptr is used in megasas_mgmt_fw_ioctl only when + * sense_len is not null, so prepare the 64bit value under + * the same condition. + */ + if (ioc->sense_len) { + sense_ptr = ioc->frame.raw + ioc->sense_off; + if (get_user(ptr, (compat_uptr_t *)sense_ptr) || + put_user(ptr, (unsigned long *)sense_ptr)) + return -EFAULT; + } + for (i = 0; i < MAX_IOCTL_SGE; i++) { if (get_user(ptr, &cioc->sgl[i].iov_base) || put_user(compat_ptr(ptr), &ioc->sgl[i].iov_base) || copy_in_user(&ioc->sgl[i].iov_len, ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] megaraid_sas: fix for 32bit apps 2010-02-05 17:04 [PATCH] megaraid_sas: fix for 32bit apps Tomas Henzl @ 2010-02-10 19:36 ` James Bottomley 2010-02-11 17:01 ` Tomas Henzl 0 siblings, 1 reply; 3+ messages in thread From: James Bottomley @ 2010-02-10 19:36 UTC (permalink / raw) To: Tomas Henzl; +Cc: linux-scsi, Yang, Bo, hch On Fri, 2010-02-05 at 18:04 +0100, Tomas Henzl wrote: > It looks like this patch - > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7b2519afa1abd1b9f63aa1e90879307842422dae > has caused a problem for 32bit programs with 64bit os - > http://bugzilla.kernel.org/show_bug.cgi?id=15001 > > megaraid: convert user space 32bit pointer > to a 64 bit one when needed. > > Signed-off-by: Tomas Henzl <thenzl@redhat.com> > > diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c > index 708ea31..7617b8e 100644 > --- a/drivers/scsi/megaraid/megaraid_sas.c > +++ b/drivers/scsi/megaraid/megaraid_sas.c > @@ -3781,6 +3781,8 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) > compat_alloc_user_space(sizeof(struct megasas_iocpacket)); > int i; > int error = 0; > + compat_uptr_t ptr; > + u8 *sense_ptr; > > if (clear_user(ioc, sizeof(*ioc))) > return -EFAULT; > @@ -3793,9 +3795,19 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) > copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32))) > return -EFAULT; > > - for (i = 0; i < MAX_IOCTL_SGE; i++) { > - compat_uptr_t ptr; > + /* > + * The sense_ptr is used in megasas_mgmt_fw_ioctl only when > + * sense_len is not null, so prepare the 64bit value under > + * the same condition. > + */ > + if (ioc->sense_len) { > + sense_ptr = ioc->frame.raw + ioc->sense_off; > + if (get_user(ptr, (compat_uptr_t *)sense_ptr) || > + put_user(ptr, (unsigned long *)sense_ptr)) This should be put_user(compat_ptr(ptr) ...) to make sure we get proper warn free promotion. For symmetry, shouldn't we be reading from cioc? I know technically the copy_in_user(ioc->frame.raw, cioc->frame.raw, 128) above did this, but it might look confusing to someone coming to the code for the first time. To be honest, I also don't think ioc->frame.raw + ioc->sense_off is legal: it's dereferencing a userspace pointer (ioc->sense_off), which will fault on non-x86 boxes, but this isn't a criticism of the patch so much as the whole driver. Bo, this looks like a security fix, because userspace can cause arbitrary data scribble currently by stuffing carefully crafted values into the four bytes after the 32 bit sense pointer, so I'd like to apply it asap. Thanks, James ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] megaraid_sas: fix for 32bit apps 2010-02-10 19:36 ` James Bottomley @ 2010-02-11 17:01 ` Tomas Henzl 0 siblings, 0 replies; 3+ messages in thread From: Tomas Henzl @ 2010-02-11 17:01 UTC (permalink / raw) To: James Bottomley; +Cc: linux-scsi, Yang, Bo, hch On 02/10/2010 08:36 PM, James Bottomley wrote: > On Fri, 2010-02-05 at 18:04 +0100, Tomas Henzl wrote: > >> It looks like this patch - >> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7b2519afa1abd1b9f63aa1e90879307842422dae >> has caused a problem for 32bit programs with 64bit os - >> http://bugzilla.kernel.org/show_bug.cgi?id=15001 >> >> megaraid: convert user space 32bit pointer >> to a 64 bit one when needed. >> >> Signed-off-by: Tomas Henzl <thenzl@redhat.com> >> >> diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c >> index 708ea31..7617b8e 100644 >> --- a/drivers/scsi/megaraid/megaraid_sas.c >> +++ b/drivers/scsi/megaraid/megaraid_sas.c >> @@ -3781,6 +3781,8 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) >> compat_alloc_user_space(sizeof(struct megasas_iocpacket)); >> int i; >> int error = 0; >> + compat_uptr_t ptr; >> + u8 *sense_ptr; >> >> if (clear_user(ioc, sizeof(*ioc))) >> return -EFAULT; >> @@ -3793,9 +3795,19 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) >> copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32))) >> return -EFAULT; >> >> - for (i = 0; i < MAX_IOCTL_SGE; i++) { >> - compat_uptr_t ptr; >> + /* >> + * The sense_ptr is used in megasas_mgmt_fw_ioctl only when >> + * sense_len is not null, so prepare the 64bit value under >> + * the same condition. >> + */ >> + if (ioc->sense_len) { >> + sense_ptr = ioc->frame.raw + ioc->sense_off; >> + if (get_user(ptr, (compat_uptr_t *)sense_ptr) || >> + put_user(ptr, (unsigned long *)sense_ptr)) >> > This should be put_user(compat_ptr(ptr) ...) to make sure we get proper > warn free promotion. > ok > For symmetry, shouldn't we be reading from cioc? I know technically the > copy_in_user(ioc->frame.raw, cioc->frame.raw, 128) above did this, but > it might look confusing to someone coming to the code for the first > time. > You are right, I just wanted to save some lines, but it could be confusing. > To be honest, I also don't think ioc->frame.raw + ioc->sense_off is > legal: it's dereferencing a userspace pointer (ioc->sense_off), which > will fault on non-x86 boxes, but this isn't a criticism of the patch so > much as the whole driver. > > Bo, this looks like a security fix, because userspace can cause > arbitrary data scribble currently by stuffing carefully crafted values > into the four bytes after the 32 bit sense pointer, so I'd like to apply > it asap. > > Thanks, > > James > New version, compile tested. megaraid: convert user space 32bit pointer to a 64 bit one when needed. Signed-off-by: Tomas Henzl <thenzl@redhat.com> diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c index 708ea31..cb70224 100644 --- a/drivers/scsi/megaraid/megaraid_sas.c +++ b/drivers/scsi/megaraid/megaraid_sas.c @@ -3781,6 +3781,8 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) compat_alloc_user_space(sizeof(struct megasas_iocpacket)); int i; int error = 0; + compat_uptr_t ptr; + u8 *sense_cioc_ptr, *sense_ioc_ptr; if (clear_user(ioc, sizeof(*ioc))) return -EFAULT; @@ -3793,9 +3795,20 @@ static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg) copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32))) return -EFAULT; - for (i = 0; i < MAX_IOCTL_SGE; i++) { - compat_uptr_t ptr; + /* + * The sense_ptr is used in megasas_mgmt_fw_ioctl only when + * sense_len is not null, so prepare the 64bit value under + * the same condition. + */ + if (ioc->sense_len) { + sense_ioc_ptr = ioc->frame.raw + ioc->sense_off; + sense_cioc_ptr = cioc->frame.raw + cioc->sense_off; + if (get_user(ptr, (compat_uptr_t *)sense_cioc_ptr) || + put_user(compat_ptr(ptr), (unsigned long *)sense_ioc_ptr)) + return -EFAULT; + } + for (i = 0; i < MAX_IOCTL_SGE; i++) { if (get_user(ptr, &cioc->sgl[i].iov_base) || put_user(compat_ptr(ptr), &ioc->sgl[i].iov_base) || copy_in_user(&ioc->sgl[i].iov_len, ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-02-11 17:02 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-05 17:04 [PATCH] megaraid_sas: fix for 32bit apps Tomas Henzl 2010-02-10 19:36 ` James Bottomley 2010-02-11 17:01 ` Tomas Henzl
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox