Skip to content

Commit

Permalink
scsi: qla2xxx: Get mutex lock before checking optrom_state
Browse files Browse the repository at this point in the history
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: [<ffffffff81326756>] memcpy+0x6/0x110
[...]
[ 1479.473673] Call Trace:
[ 1479.474296]  [<ffffffff81225cbc>] ? memory_read_from_buffer+0x3c/0x60
[ 1479.474941]  [<ffffffffa01574dc>] qla2x00_sysfs_read_optrom+0x9c/0xc0 [qla2xxx]
[ 1479.475571]  [<ffffffff8127e76b>] read+0xdb/0x1f0
[ 1479.476206]  [<ffffffff811fdf9e>] vfs_read+0x9e/0x170
[ 1479.476839]  [<ffffffff811feb6f>] SyS_read+0x7f/0xe0
[ 1479.477466]  [<ffffffff816964c9>] 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 <mgandhi@redhat.com>
Signed-off-by: Milan P. Gandhi <mgandhi@redhat.com>
Reviewed-by: Laurence Oberman <loberman@redhat.com>
Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
  • Loading branch information
Milan P. Gandhi authored and Martin K. Petersen committed Jan 10, 2017
1 parent 2d1148f commit c7702b8
Showing 1 changed file with 13 additions and 5 deletions.
18 changes: 13 additions & 5 deletions drivers/scsi/qla2xxx/qla_attr.c
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,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;
Expand All @@ -263,14 +266,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);

Expand Down

0 comments on commit c7702b8

Please sign in to comment.