| Message ID | 288dbcf8-29b5-8156-aba8-049edfed1dde@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
----- Original Message ----- > From: "Milan P. Gandhi" <mgandhi@redhat.com> > To: linux-scsi@vger.kernel.org > Cc: "Laurence Oberman" <loberman@redhat.com>, "chad dupuis" <chad.dupuis@cavium.com> > 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: [<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> > --- > 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 <loberman@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Milan" == Milan P Gandhi <mgandhi@redhat.com> 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?
SGkgTWlsYW4sDQoNCg0KDQpPbiAxMi8yNC8xNiwgODozMiBBTSwgImxpbnV4LXNjc2ktb3duZXJA dmdlci5rZXJuZWwub3JnIG9uIGJlaGFsZiBvZiBNaWxhbiBQLiBHYW5kaGkiIDxsaW51eC1zY3Np LW93bmVyQHZnZXIua2VybmVsLm9yZyBvbiBiZWhhbGYgb2YgbWdhbmRoaUByZWRoYXQuY29tPiB3 cm90ZToNCg0KPkhlbGxvLA0KPg0KPlRoZXJlIGlzIGEgcmFjZSBjb25kaXRpb24gd2l0aCBxbGEy eHh4IG9wdHJvbSBmdW5jdGlvbnMgd2hlcmUNCj5vbmUgdGhyZWFkIG1pZ2h0IG1vZGlmeSBvcHRy b20gYnVmZmVyLCBvcHRyb21fc3RhdGUgd2hpbGUgDQo+b3RoZXIgdGhyZWFkIGlzIHN0aWxsIHJl YWRpbmcgZnJvbSBpdC4NCj4NCj5JbiBjb3VwbGUgb2YgY3Jhc2hlcywgaXQgd2FzIGZvdW5kIHRo YXQgd2UgaGFkIHN1Y2Nlc3NmdWxseSANCj5wYXNzZWQgdGhlIGZvbGxvd2luZyAnaWYnIGNoZWNr IHdoZXJlIHdlIGNvbmZpcm0gb3B0cm9tX3N0YXRlIA0KPnRvIGJlIFFMQV9TUkVBRElORy4gQnV0 IGJ5IHRoZSB0aW1lIHdlIGFjcXVpcmVkIG11dGV4IGxvY2sgDQo+dG8gcHJvY2VlZCB3aXRoIG1l bW9yeV9yZWFkX2Zyb21fYnVmZmVyIGZ1bmN0aW9uLCBzb21lIG90aGVyIA0KPnRocmVhZC9wcm9j ZXNzIGhhZCBhbHJlYWR5IG1vZGlmaWVkIHRoYXQgb3B0aW9uIHJvbSBidWZmZXIgIA0KPmFuZCBv cHRyb21fc3RhdGUgZnJvbSBRTEFfU1JFQURJTkcgdG8gUUxBX1NXQUlUSU5HLiBUaGVuIA0KPndl IGdvdCBoYS0+b3B0cm9tX2J1ZmZlciAweDAgYW5kIGNyYXNoZWQgdGhlIHN5c3RlbTogDQo+DQo+ ICAgICAgICBpZiAoaGEtPm9wdHJvbV9zdGF0ZSAhPSBRTEFfU1JFQURJTkcpDQo+ICAgICAgICAg ICAgICAgIHJldHVybiAwOw0KPg0KPiAgICAgICAgbXV0ZXhfbG9jaygmaGEtPm9wdHJvbV9tdXRl eCk7DQo+ICAgICAgICBydmFsID0gbWVtb3J5X3JlYWRfZnJvbV9idWZmZXIoYnVmLCBjb3VudCwg Jm9mZiwgaGEtPm9wdHJvbV9idWZmZXIsDQo+ICAgICAgICAgICAgaGEtPm9wdHJvbV9yZWdpb25f c2l6ZSk7DQo+ICAgICAgICBtdXRleF91bmxvY2soJmhhLT5vcHRyb21fbXV0ZXgpOw0KPg0KPg0K PldpdGggY3VycmVudCBvcHRyb20gZnVuY3Rpb24gd2UgZ2V0IGZvbGxvd2luZyBjcmFzaCBkdWUg dG8gDQo+YSByYWNlIGNvbmRpdGlvbjoNCj4NCj5bIDE0NzkuNDY2Njc5XSBCVUc6IHVuYWJsZSB0 byBoYW5kbGUga2VybmVsIE5VTEwgcG9pbnRlciBkZXJlZmVyZW5jZSBhdCAgICAgICAgICAgKG51 bGwpDQo+WyAxNDc5LjQ2NjcwN10gSVA6IFs8ZmZmZmZmZmY4MTMyNjc1Nj5dIG1lbWNweSsweDYv MHgxMTANCj5bLi4uXQ0KPlsgMTQ3OS40NzM2NzNdIENhbGwgVHJhY2U6DQo+WyAxNDc5LjQ3NDI5 Nl0gIFs8ZmZmZmZmZmY4MTIyNWNiYz5dID8gbWVtb3J5X3JlYWRfZnJvbV9idWZmZXIrMHgzYy8w eDYwDQo+WyAxNDc5LjQ3NDk0MV0gIFs8ZmZmZmZmZmZhMDE1NzRkYz5dIHFsYTJ4MDBfc3lzZnNf cmVhZF9vcHRyb20rMHg5Yy8weGMwIFtxbGEyeHh4XQ0KPlsgMTQ3OS40NzU1NzFdICBbPGZmZmZm ZmZmODEyN2U3NmI+XSByZWFkKzB4ZGIvMHgxZjANCj5bIDE0NzkuNDc2MjA2XSAgWzxmZmZmZmZm ZjgxMWZkZjllPl0gdmZzX3JlYWQrMHg5ZS8weDE3MA0KPlsgMTQ3OS40NzY4MzldICBbPGZmZmZm ZmZmODExZmViNmY+XSBTeVNfcmVhZCsweDdmLzB4ZTANCj5bIDE0NzkuNDc3NDY2XSAgWzxmZmZm ZmZmZjgxNjk2NGM5Pl0gc3lzdGVtX2NhbGxfZmFzdHBhdGgrMHgxNi8weDFiDQo+DQo+DQo+QmVs b3cgcGF0Y2ggbW9kaWZpZXMgcWxhMngwMF9zeXNmc19yZWFkX29wdHJvbSwNCj5xbGEyeDAwX3N5 c2ZzX3dyaXRlX29wdHJvbSBmdW5jdGlvbnMgdG8gZ2V0IHRoZSBtdXRleF9sb2NrIA0KPmJlZm9y ZSBjaGVja2luZyBoYS0+b3B0cm9tX3N0YXRlIHRvIGF2b2lkIHNpbWlsYXIgY3Jhc2hlcy4NCj4N Cj5UaGUgcGF0Y2ggd2FzIGFwcGxpZWQgYW5kIHRlc3RlZCBhbmQgc2FtZSBjcmFzaGVzIHdlcmUg bm8gDQo+bG9uZ2VyIG9ic2VydmVkIGFnYWluLg0KPg0KPg0KPlRlc3RlZC1ieTogTWlsYW4gUC4g R2FuZGhpIDxtZ2FuZGhpQHJlZGhhdC5jb20+DQo+U2lnbmVkLW9mZi1ieTogTWlsYW4gUC4gR2Fu ZGhpIDxtZ2FuZGhpQHJlZGhhdC5jb20+DQo+LS0tDQo+IGRyaXZlcnMvc2NzaS9xbGEyeHh4L3Fs YV9hdHRyLmMgfCAxOCArKysrKysrKysrKysrLS0tLS0NCj4gMSBmaWxlIGNoYW5nZWQsIDEzIGlu c2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0pDQo+DQo+ZGlmZiAtLWdpdCBhL2RyaXZlcnMvc2Nz aS9xbGEyeHh4L3FsYV9hdHRyLmMgYi9kcml2ZXJzL3Njc2kvcWxhMnh4eC9xbGFfYXR0ci5jDQo+ aW5kZXggZGE1YWUxMS4uNDdlYTE2NCAxMDA2NDQNCj4tLS0gYS9kcml2ZXJzL3Njc2kvcWxhMnh4 eC9xbGFfYXR0ci5jDQo+KysrIGIvZHJpdmVycy9zY3NpL3FsYTJ4eHgvcWxhX2F0dHIuYw0KPkBA IC0zMjksMTIgKzMyOSwxNSBAQCBxbGEyeDAwX3N5c2ZzX3JlYWRfb3B0cm9tKHN0cnVjdCBmaWxl ICpmaWxwLCBzdHJ1Y3Qga29iamVjdCAqa29iaiwNCj4gCXN0cnVjdCBxbGFfaHdfZGF0YSAqaGEg PSB2aGEtPmh3Ow0KPiAJc3NpemVfdCBydmFsID0gMDsNCj4gDQo+KwltdXRleF9sb2NrKCZoYS0+ b3B0cm9tX211dGV4KTsNCj4rDQo+IAlpZiAoaGEtPm9wdHJvbV9zdGF0ZSAhPSBRTEFfU1JFQURJ TkcpDQo+LQkJcmV0dXJuIDA7DQo+KwkJZ290byBvdXQ7DQo+IA0KPi0JbXV0ZXhfbG9jaygmaGEt Pm9wdHJvbV9tdXRleCk7DQo+IAlydmFsID0gbWVtb3J5X3JlYWRfZnJvbV9idWZmZXIoYnVmLCBj b3VudCwgJm9mZiwgaGEtPm9wdHJvbV9idWZmZXIsDQo+IAkgICAgaGEtPm9wdHJvbV9yZWdpb25f c2l6ZSk7DQo+Kw0KPitvdXQ6DQo+IAltdXRleF91bmxvY2soJmhhLT5vcHRyb21fbXV0ZXgpOw0K PiANCj4gCXJldHVybiBydmFsOw0KPkBAIC0zNDksMTQgKzM1MiwxOSBAQCBxbGEyeDAwX3N5c2Zz X3dyaXRlX29wdHJvbShzdHJ1Y3QgZmlsZSAqZmlscCwgc3RydWN0IGtvYmplY3QgKmtvYmosDQo+ IAkgICAgc3RydWN0IGRldmljZSwga29iaikpKTsNCj4gCXN0cnVjdCBxbGFfaHdfZGF0YSAqaGEg PSB2aGEtPmh3Ow0KPiANCj4tCWlmIChoYS0+b3B0cm9tX3N0YXRlICE9IFFMQV9TV1JJVElORykN Cj4rCW11dGV4X2xvY2soJmhhLT5vcHRyb21fbXV0ZXgpOw0KPisNCj4rCWlmIChoYS0+b3B0cm9t X3N0YXRlICE9IFFMQV9TV1JJVElORykgew0KPisJCW11dGV4X3VubG9jaygmaGEtPm9wdHJvbV9t dXRleCk7DQo+IAkJcmV0dXJuIC1FSU5WQUw7DQo+LQlpZiAob2ZmID4gaGEtPm9wdHJvbV9yZWdp b25fc2l6ZSkNCj4rCX0NCj4rCWlmIChvZmYgPiBoYS0+b3B0cm9tX3JlZ2lvbl9zaXplKSB7DQo+ KwkJbXV0ZXhfdW5sb2NrKCZoYS0+b3B0cm9tX211dGV4KTsNCj4gCQlyZXR1cm4gLUVSQU5HRTsN Cj4rCX0NCj4gCWlmIChvZmYgKyBjb3VudCA+IGhhLT5vcHRyb21fcmVnaW9uX3NpemUpDQo+IAkJ Y291bnQgPSBoYS0+b3B0cm9tX3JlZ2lvbl9zaXplIC0gb2ZmOw0KPiANCj4tCW11dGV4X2xvY2so JmhhLT5vcHRyb21fbXV0ZXgpOw0KPiAJbWVtY3B5KCZoYS0+b3B0cm9tX2J1ZmZlcltvZmZdLCBi dWYsIGNvdW50KTsNCj4gCW11dGV4X3VubG9jaygmaGEtPm9wdHJvbV9tdXRleCk7DQo+IA0KPi0t DQoNClRoYW5rcyBmb3IgdGhlIFBhdGNoLiBUaGlzIGxvb2tzIGdvb2QuDQoNCkFja2VkLWJ5OiBI aW1hbnNodSBNYWRoYW5pIDxoaW1hbnNodS5tYWRoYW5pQGNhdml1bS5jb20+DQoNCj5UbyB1bnN1 YnNjcmliZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgt c2NzaSIgaW4NCj50aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVs Lm9yZw0KPk1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFq b3Jkb21vLWluZm8uaHRtbA0K -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Milan" == Milan P Gandhi <mgandhi@redhat.com> 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.
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);
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: [<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> --- drivers/scsi/qla2xxx/qla_attr.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html