[bug report] skd: Avoid that module unloading triggers a use-after-free

[bug report] skd: Avoid that module unloading triggers a use-after-free

[Dan Carpenter]

Hello Bart Van Assche,

This is a semi-automatic email about new static checker warnings.

The patch 7277cc67b391: "skd: Avoid that module unloading triggers a 
use-after-free" from Aug 17, 2017, leads to the following Smatch 
complaint:

drivers/block/skd_main.c:3080 skd_free_disk()
	 error: we previously assumed 'disk' could be null (see line 3074)

drivers/block/skd_main.c
  3073	
  3074		if (disk && (disk->flags & GENHD_FL_UP))
                    ^^^^
Existing code checked for NULL.  The new code shuffles things around.

  3075			del_gendisk(disk);
  3076	
  3077		if (skdev->queue) {
  3078			blk_cleanup_queue(skdev->queue);
  3079			skdev->queue = NULL;
  3080			disk->queue = NULL;
                        ^^^^^^^^^^^
Now we don't check here.

  3081		}
  3082	

regards,
dan carpenter

Re: [bug report] skd: Avoid that module unloading triggers a use-after-free

[Bart Van Assche]

T24gVGh1LCAyMDE3LTA4LTI0IGF0IDE0OjA0ICswMzAwLCBEYW4gQ2FycGVudGVyIHdyb3RlOg0K
PiBIZWxsbyBCYXJ0IFZhbiBBc3NjaGUsDQo+IA0KPiBUaGlzIGlzIGEgc2VtaS1hdXRvbWF0aWMg
ZW1haWwgYWJvdXQgbmV3IHN0YXRpYyBjaGVja2VyIHdhcm5pbmdzLg0KPiANCj4gVGhlIHBhdGNo
IDcyNzdjYzY3YjM5MTogInNrZDogQXZvaWQgdGhhdCBtb2R1bGUgdW5sb2FkaW5nIHRyaWdnZXJz
IGEgDQo+IHVzZS1hZnRlci1mcmVlIiBmcm9tIEF1ZyAxNywgMjAxNywgbGVhZHMgdG8gdGhlIGZv
bGxvd2luZyBTbWF0Y2ggDQo+IGNvbXBsYWludDoNCj4gDQo+IGRyaXZlcnMvYmxvY2svc2tkX21h
aW4uYzozMDgwIHNrZF9mcmVlX2Rpc2soKQ0KPiAJIGVycm9yOiB3ZSBwcmV2aW91c2x5IGFzc3Vt
ZWQgJ2Rpc2snIGNvdWxkIGJlIG51bGwgKHNlZSBsaW5lIDMwNzQpDQo+IA0KPiBkcml2ZXJzL2Js
b2NrL3NrZF9tYWluLmMNCj4gICAzMDczCQ0KPiAgIDMwNzQJCWlmIChkaXNrICYmIChkaXNrLT5m
bGFncyAmIEdFTkhEX0ZMX1VQKSkNCj4gICAgICAgICAgICAgICAgICAgICBeXl5eDQo+IEV4aXN0
aW5nIGNvZGUgY2hlY2tlZCBmb3IgTlVMTC4gIFRoZSBuZXcgY29kZSBzaHVmZmxlcyB0aGluZ3Mg
YXJvdW5kLg0KPiANCj4gICAzMDc1CQkJZGVsX2dlbmRpc2soZGlzayk7DQo+ICAgMzA3NgkNCj4g
ICAzMDc3CQlpZiAoc2tkZXYtPnF1ZXVlKSB7DQo+ICAgMzA3OAkJCWJsa19jbGVhbnVwX3F1ZXVl
KHNrZGV2LT5xdWV1ZSk7DQo+ICAgMzA3OQkJCXNrZGV2LT5xdWV1ZSA9IE5VTEw7DQo+ICAgMzA4
MAkJCWRpc2stPnF1ZXVlID0gTlVMTDsNCj4gICAgICAgICAgICAgICAgICAgICAgICAgXl5eXl5e
Xl5eXl4NCj4gTm93IHdlIGRvbid0IGNoZWNrIGhlcmUuDQo+IA0KPiAgIDMwODEJCX0NCj4gICAz
MDgyCQ0KPiANCj4gcmVnYXJkcywNCj4gZGFuIGNhcnBlbnRlcg0KDQpIZWxsbyBEYW4sDQoNCklm
IHlvdSBoYXZlIGEgbG9vayBhdCBza2RfY29uc19kaXNrKCkgeW91IHdpbGwgc2VlIHRoYXQgc2tk
ZXYtPnF1ZXVlICE9IE5VTEwNCmltcGxpZXMgdGhhdCBza2Rldi0+ZGlzayAhPSBOVUxMLiBTbyBJ
IHRoaW5rIHRoZSBhYm92ZSByZXBvcnQgaXMgYSBmYWxzZQ0KcG9zaXRpdmUuDQoNCkJhcnQu

Re: [bug report] skd: Avoid that module unloading triggers a use-after-free

[Dan Carpenter]

On Thu, Aug 24, 2017 at 03:04:12PM +0000, Bart Van Assche wrote:
> On Thu, 2017-08-24 at 14:04 +0300, Dan Carpenter wrote:
> > Hello Bart Van Assche,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch 7277cc67b391: "skd: Avoid that module unloading triggers a 
> > use-after-free" from Aug 17, 2017, leads to the following Smatch 
> > complaint:
> > 
> > drivers/block/skd_main.c:3080 skd_free_disk()
> > 	 error: we previously assumed 'disk' could be null (see line 3074)
> > 
> > drivers/block/skd_main.c
> >   3073	
> >   3074		if (disk && (disk->flags & GENHD_FL_UP))
> >                     ^^^^
> > Existing code checked for NULL.  The new code shuffles things around.
> > 
> >   3075			del_gendisk(disk);
> >   3076	
> >   3077		if (skdev->queue) {
> >   3078			blk_cleanup_queue(skdev->queue);
> >   3079			skdev->queue = NULL;
> >   3080			disk->queue = NULL;
> >                         ^^^^^^^^^^^
> > Now we don't check here.
> > 
> >   3081		}
> >   3082	
> > 
> > regards,
> > dan carpenter
> 
> Hello Dan,
> 
> If you have a look at skd_cons_disk() you will see that skdev->queue != NULL
> implies that skdev->disk != NULL. So I think the above report is a false
> positive.
> 

Oh, yeah.  You're right.  Thanks for taking a look at this.

regards,
dan carpenter

Re: [bug report] skd: Avoid that module unloading triggers a use-after-free

[Bart Van Assche]

T24gVGh1LCAyMDE3LTA4LTI0IGF0IDIxOjM2ICswMzAwLCBEYW4gQ2FycGVudGVyIHdyb3RlOg0K
PiBPbiBUaHUsIEF1ZyAyNCwgMjAxNyBhdCAwMzowNDoxMlBNICswMDAwLCBCYXJ0IFZhbiBBc3Nj
aGUgd3JvdGU6DQo+ID4gSWYgeW91IGhhdmUgYSBsb29rIGF0IHNrZF9jb25zX2Rpc2soKSB5b3Ug
d2lsbCBzZWUgdGhhdCBza2Rldi0+cXVldWUgIT0gTlVMTA0KPiA+IGltcGxpZXMgdGhhdCBza2Rl
di0+ZGlzayAhPSBOVUxMLiBTbyBJIHRoaW5rIHRoZSBhYm92ZSByZXBvcnQgaXMgYSBmYWxzZQ0K
PiA+IHBvc2l0aXZlLg0KPiANCj4gT2gsIHllYWguICBZb3UncmUgcmlnaHQuICBUaGFua3MgZm9y
IHRha2luZyBhIGxvb2sgYXQgdGhpcy4NCg0KVGhhbmsgeW91IGZvciBhbGwgdGhlIHdvcmsgeW91
IGhhdmUgZG9uZSBhbmQgYXJlIHN0aWxsIGRvaW5nIG9uIHNtYXRjaCBhbmQgb24NCnZlcmlmeWlu
ZyBhbGwgbmV3IGNvZGUhDQoNCkJhcnQu