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