Re: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
> "Milan" == Milan P Gandhi writes: Milan> There is a race condition with qla2xxx optrom functions where one Milan> thread might modify optrom buffer, optrom_state while other Milan> thread is still reading from it. Applied to 4.10/scsi-fixes. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
Hi Milan,
On 12/24/16, 8:32 AM, "linux-scsi-ow...@vger.kernel.org on behalf of Milan P.
Gandhi"
wrote:
>Hello,
>
>There is a race condition with qla2xxx optrom functions where
>one thread might modify optrom buffer, optrom_state while
>other thread is still reading from it.
>
>In couple of crashes, it was found that we had successfully
>passed the following 'if' check where we confirm optrom_state
>to be QLA_SREADING. But by the time we acquired mutex lock
>to proceed with memory_read_from_buffer function, some other
>thread/process had already modified that option rom buffer
>and optrom_state from QLA_SREADING to QLA_SWAITING. Then
>we got ha->optrom_buffer 0x0 and crashed the system:
>
>if (ha->optrom_state != QLA_SREADING)
>return 0;
>
>mutex_lock(&ha->optrom_mutex);
>rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
>ha->optrom_region_size);
>mutex_unlock(&ha->optrom_mutex);
>
>
>With current optrom function we get following crash due to
>a race condition:
>
>[ 1479.466679] BUG: unable to handle kernel NULL pointer dereference at
> (null)
>[ 1479.466707] IP: [] memcpy+0x6/0x110
>[...]
>[ 1479.473673] Call Trace:
>[ 1479.474296] [] ? memory_read_from_buffer+0x3c/0x60
>[ 1479.474941] [] qla2x00_sysfs_read_optrom+0x9c/0xc0
>[qla2xxx]
>[ 1479.475571] [] read+0xdb/0x1f0
>[ 1479.476206] [] vfs_read+0x9e/0x170
>[ 1479.476839] [] SyS_read+0x7f/0xe0
>[ 1479.477466] [] system_call_fastpath+0x16/0x1b
>
>
>Below patch modifies qla2x00_sysfs_read_optrom,
>qla2x00_sysfs_write_optrom functions to get the mutex_lock
>before checking ha->optrom_state to avoid similar crashes.
>
>The patch was applied and tested and same crashes were no
>longer observed again.
>
>
>Tested-by: Milan P. Gandhi
>Signed-off-by: Milan P. Gandhi
>---
> drivers/scsi/qla2xxx/qla_attr.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
>index da5ae11..47ea164 100644
>--- a/drivers/scsi/qla2xxx/qla_attr.c
>+++ b/drivers/scsi/qla2xxx/qla_attr.c
>@@ -329,12 +329,15 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct
>kobject *kobj,
> struct qla_hw_data *ha = vha->hw;
> ssize_t rval = 0;
>
>+ mutex_lock(&ha->optrom_mutex);
>+
> if (ha->optrom_state != QLA_SREADING)
>- return 0;
>+ goto out;
>
>- mutex_lock(&ha->optrom_mutex);
> rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
> ha->optrom_region_size);
>+
>+out:
> mutex_unlock(&ha->optrom_mutex);
>
> return rval;
>@@ -349,14 +352,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct
>kobject *kobj,
> struct device, kobj)));
> struct qla_hw_data *ha = vha->hw;
>
>- if (ha->optrom_state != QLA_SWRITING)
>+ mutex_lock(&ha->optrom_mutex);
>+
>+ if (ha->optrom_state != QLA_SWRITING) {
>+ mutex_unlock(&ha->optrom_mutex);
> return -EINVAL;
>- if (off > ha->optrom_region_size)
>+ }
>+ if (off > ha->optrom_region_size) {
>+ mutex_unlock(&ha->optrom_mutex);
> return -ERANGE;
>+ }
> if (off + count > ha->optrom_region_size)
> count = ha->optrom_region_size - off;
>
>- mutex_lock(&ha->optrom_mutex);
> memcpy(&ha->optrom_buffer[off], buf, count);
> mutex_unlock(&ha->optrom_mutex);
>
>--
Thanks for the Patch. This looks good.
Acked-by: Himanshu Madhani
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
N�r��yb�X��ǧv�^�){.n�+{���"�{ay� ʇڙ�,j��f���h���z� �w���
���j:+v���w�j�mzZ+�ݢj"��!�i
Re: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
> "Milan" == Milan P Gandhi writes: Milan> Hello, There is a race condition with qla2xxx optrom functions Milan> where one thread might modify optrom buffer, optrom_state while Milan> other thread is still reading from it. Chad? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
- Original Message -
> From: "Milan P. Gandhi"
> To: linux-scsi@vger.kernel.org
> Cc: "Laurence Oberman" , "chad dupuis"
>
> Sent: Saturday, December 24, 2016 11:32:46 AM
> Subject: [PATCH] qla2xxx: Get mutex lock before checking optrom_state
>
> Hello,
>
> There is a race condition with qla2xxx optrom functions where
> one thread might modify optrom buffer, optrom_state while
> other thread is still reading from it.
>
> In couple of crashes, it was found that we had successfully
> passed the following 'if' check where we confirm optrom_state
> to be QLA_SREADING. But by the time we acquired mutex lock
> to proceed with memory_read_from_buffer function, some other
> thread/process had already modified that option rom buffer
> and optrom_state from QLA_SREADING to QLA_SWAITING. Then
> we got ha->optrom_buffer 0x0 and crashed the system:
>
> if (ha->optrom_state != QLA_SREADING)
> return 0;
>
> mutex_lock(&ha->optrom_mutex);
> rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
> ha->optrom_region_size);
> mutex_unlock(&ha->optrom_mutex);
>
>
> With current optrom function we get following crash due to
> a race condition:
>
> [ 1479.466679] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [ 1479.466707] IP: [] memcpy+0x6/0x110
> [...]
> [ 1479.473673] Call Trace:
> [ 1479.474296] [] ? memory_read_from_buffer+0x3c/0x60
> [ 1479.474941] [] qla2x00_sysfs_read_optrom+0x9c/0xc0
> [qla2xxx]
> [ 1479.475571] [] read+0xdb/0x1f0
> [ 1479.476206] [] vfs_read+0x9e/0x170
> [ 1479.476839] [] SyS_read+0x7f/0xe0
> [ 1479.477466] [] system_call_fastpath+0x16/0x1b
>
>
> Below patch modifies qla2x00_sysfs_read_optrom,
> qla2x00_sysfs_write_optrom functions to get the mutex_lock
> before checking ha->optrom_state to avoid similar crashes.
>
> The patch was applied and tested and same crashes were no
> longer observed again.
>
>
> Tested-by: Milan P. Gandhi
> Signed-off-by: Milan P. Gandhi
> ---
> drivers/scsi/qla2xxx/qla_attr.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c
> b/drivers/scsi/qla2xxx/qla_attr.c
> index da5ae11..47ea164 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -329,12 +329,15 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct
> kobject *kobj,
> struct qla_hw_data *ha = vha->hw;
> ssize_t rval = 0;
>
> + mutex_lock(&ha->optrom_mutex);
> +
> if (ha->optrom_state != QLA_SREADING)
> - return 0;
> + goto out;
>
> - mutex_lock(&ha->optrom_mutex);
> rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
> ha->optrom_region_size);
> +
> +out:
> mutex_unlock(&ha->optrom_mutex);
>
> return rval;
> @@ -349,14 +352,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct
> kobject *kobj,
> struct device, kobj)));
> struct qla_hw_data *ha = vha->hw;
>
> - if (ha->optrom_state != QLA_SWRITING)
> + mutex_lock(&ha->optrom_mutex);
> +
> + if (ha->optrom_state != QLA_SWRITING) {
> + mutex_unlock(&ha->optrom_mutex);
> return -EINVAL;
> - if (off > ha->optrom_region_size)
> + }
> + if (off > ha->optrom_region_size) {
> + mutex_unlock(&ha->optrom_mutex);
> return -ERANGE;
> + }
> if (off + count > ha->optrom_region_size)
> count = ha->optrom_region_size - off;
>
> - mutex_lock(&ha->optrom_mutex);
> memcpy(&ha->optrom_buffer[off], buf, count);
> mutex_unlock(&ha->optrom_mutex);
>
>
Looks good, and I know it fixed the issue.
Milan, Thank you for this work.
Reviewed-by: Laurence Oberman
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] qla2xxx: Get mutex lock before checking optrom_state
Hello,
There is a race condition with qla2xxx optrom functions where
one thread might modify optrom buffer, optrom_state while
other thread is still reading from it.
In couple of crashes, it was found that we had successfully
passed the following 'if' check where we confirm optrom_state
to be QLA_SREADING. But by the time we acquired mutex lock
to proceed with memory_read_from_buffer function, some other
thread/process had already modified that option rom buffer
and optrom_state from QLA_SREADING to QLA_SWAITING. Then
we got ha->optrom_buffer 0x0 and crashed the system:
if (ha->optrom_state != QLA_SREADING)
return 0;
mutex_lock(&ha->optrom_mutex);
rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
ha->optrom_region_size);
mutex_unlock(&ha->optrom_mutex);
With current optrom function we get following crash due to
a race condition:
[ 1479.466679] BUG: unable to handle kernel NULL pointer dereference at
(null)
[ 1479.466707] IP: [] memcpy+0x6/0x110
[...]
[ 1479.473673] Call Trace:
[ 1479.474296] [] ? memory_read_from_buffer+0x3c/0x60
[ 1479.474941] [] qla2x00_sysfs_read_optrom+0x9c/0xc0
[qla2xxx]
[ 1479.475571] [] read+0xdb/0x1f0
[ 1479.476206] [] vfs_read+0x9e/0x170
[ 1479.476839] [] SyS_read+0x7f/0xe0
[ 1479.477466] [] system_call_fastpath+0x16/0x1b
Below patch modifies qla2x00_sysfs_read_optrom,
qla2x00_sysfs_write_optrom functions to get the mutex_lock
before checking ha->optrom_state to avoid similar crashes.
The patch was applied and tested and same crashes were no
longer observed again.
Tested-by: Milan P. Gandhi
Signed-off-by: Milan P. Gandhi
---
drivers/scsi/qla2xxx/qla_attr.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index da5ae11..47ea164 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -329,12 +329,15 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct
kobject *kobj,
struct qla_hw_data *ha = vha->hw;
ssize_t rval = 0;
+ mutex_lock(&ha->optrom_mutex);
+
if (ha->optrom_state != QLA_SREADING)
- return 0;
+ goto out;
- mutex_lock(&ha->optrom_mutex);
rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
ha->optrom_region_size);
+
+out:
mutex_unlock(&ha->optrom_mutex);
return rval;
@@ -349,14 +352,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct
kobject *kobj,
struct device, kobj)));
struct qla_hw_data *ha = vha->hw;
- if (ha->optrom_state != QLA_SWRITING)
+ mutex_lock(&ha->optrom_mutex);
+
+ if (ha->optrom_state != QLA_SWRITING) {
+ mutex_unlock(&ha->optrom_mutex);
return -EINVAL;
- if (off > ha->optrom_region_size)
+ }
+ if (off > ha->optrom_region_size) {
+ mutex_unlock(&ha->optrom_mutex);
return -ERANGE;
+ }
if (off + count > ha->optrom_region_size)
count = ha->optrom_region_size - off;
- mutex_lock(&ha->optrom_mutex);
memcpy(&ha->optrom_buffer[off], buf, count);
mutex_unlock(&ha->optrom_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
