Skip to content

Commit c7702b8

Browse files
mpg-rhmartinkpetersen
authored andcommitted
scsi: qla2xxx: Get mutex lock before checking optrom_state
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 <[email protected]> Signed-off-by: Milan P. Gandhi <[email protected]> Reviewed-by: Laurence Oberman <[email protected]> Acked-by: Himanshu Madhani <[email protected]> Signed-off-by: Martin K. Petersen <[email protected]>
1 parent 2d1148f commit c7702b8

File tree

1 file changed

+13
-5
lines changed

1 file changed

+13
-5
lines changed

drivers/scsi/qla2xxx/qla_attr.c

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

246+
mutex_lock(&ha->optrom_mutex);
247+
246248
if (ha->optrom_state != QLA_SREADING)
247-
return 0;
249+
goto out;
248250

249-
mutex_lock(&ha->optrom_mutex);
250251
rval = memory_read_from_buffer(buf, count, &off, ha->optrom_buffer,
251252
ha->optrom_region_size);
253+
254+
out:
252255
mutex_unlock(&ha->optrom_mutex);
253256

254257
return rval;
@@ -263,14 +266,19 @@ qla2x00_sysfs_write_optrom(struct file *filp, struct kobject *kobj,
263266
struct device, kobj)));
264267
struct qla_hw_data *ha = vha->hw;
265268

266-
if (ha->optrom_state != QLA_SWRITING)
269+
mutex_lock(&ha->optrom_mutex);
270+
271+
if (ha->optrom_state != QLA_SWRITING) {
272+
mutex_unlock(&ha->optrom_mutex);
267273
return -EINVAL;
268-
if (off > ha->optrom_region_size)
274+
}
275+
if (off > ha->optrom_region_size) {
276+
mutex_unlock(&ha->optrom_mutex);
269277
return -ERANGE;
278+
}
270279
if (off + count > ha->optrom_region_size)
271280
count = ha->optrom_region_size - off;
272281

273-
mutex_lock(&ha->optrom_mutex);
274282
memcpy(&ha->optrom_buffer[off], buf, count);
275283
mutex_unlock(&ha->optrom_mutex);
276284

0 commit comments

Comments
 (0)