Skip to content

Commit 1e43b2d

Browse files
mpg-rhgregkh
authored andcommitted
scsi: qla2xxx: Get mutex lock before checking optrom_state
[ Upstream commit c7702b8c22712a06080e10f1d2dee1a133ec8809 ] 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> Signed-off-by: Sasha Levin <alexander.levin@verizon.com> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent a987371 commit 1e43b2d

1 file changed

Lines changed: 13 additions & 5 deletions

File tree

drivers/scsi/qla2xxx/qla_attr.c

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -329,12 +329,15 @@ qla2x00_sysfs_read_optrom(struct file *filp, struct kobject *kobj,
329329
struct qla_hw_data *ha = vha->hw;
330330
ssize_t rval = 0;
331331

332+
mutex_lock(&ha->optrom_mutex);
333+
332334
if (ha->optrom_state != QLA_SREADING)
333-
return 0;
335+
goto out;
334336

335-
mutex_lock(&ha->optrom_mutex);
336337
rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
337338
ha->optrom_region_size);
339+
340+
out:
338341
mutex_unlock(&ha->optrom_mutex);
339342

340343
return rval;
@@ -349,14 +352,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct kobject *kobj,
349352
struct device, kobj)));
350353
struct qla_hw_data *ha = vha->hw;
351354

352-
if (ha->optrom_state != QLA_SWRITING)
355+
mutex_lock(&ha->optrom_mutex);
356+
357+
if (ha->optrom_state != QLA_SWRITING) {
358+
mutex_unlock(&ha->optrom_mutex);
353359
return -EINVAL;
354-
if (off > ha->optrom_region_size)
360+
}
361+
if (off > ha->optrom_region_size) {
362+
mutex_unlock(&ha->optrom_mutex);
355363
return -ERANGE;
364+
}
356365
if (off + count > ha->optrom_region_size)
357366
count = ha->optrom_region_size - off;
358367

359-
mutex_lock(&ha->optrom_mutex);
360368
memcpy(&ha->optrom_buffer[off], buf, count);
361369
mutex_unlock(&ha->optrom_mutex);
362370

0 commit comments

Comments
 (0)