Skip to contentSkip to conversation

media-entity: protect object creation/removal using spin lock

media Dan Carpenter 2 participants 4 messages Dec 14, 2015 Media (V4L/DVB)

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Mauro Carvalho Chehab,

The patch f8fd4c61b5ae: "[media] media-entity: protect object creation/removal using spin lock" from Dec 9, 2015, leads to the following static checker warning:

drivers/media/media-entity.c:781 media_remove_intf_link()
error: dereferencing freed memory 'link'

drivers/media/media-entity.c
777 void media_remove_intf_link(struct media_link *link) 778 {

   779          spin_lock(&link->graph_obj.mdev->lock);
   780          __media_remove_intf_link(link);
   781          spin_unlock(&link->graph_obj.mdev->lock);

Do we need this unlock any more? Haven't we freed the lock on the previous line?

782 }

regards,
dan carpenter

#1 lore raw

Em Mon, 14 Dec 2015 22:50:53 +0300
Dan Carpenter <[email protected]> escreveu:

Quoted 9 lines

Hello Mauro Carvalho Chehab,

The patch f8fd4c61b5ae: "[media] media-entity: protect object creation/removal using spin lock" from Dec 9, 2015, leads to the following static checker warning:

drivers/media/media-entity.c:781 media_remove_intf_link()
error: dereferencing freed memory 'link'

Quoted 6 lines

drivers/media/media-entity.c
777 void media_remove_intf_link(struct media_link *link)

   778  {
   779          spin_lock(&link->graph_obj.mdev->lock);
   780          __media_remove_intf_link(link);
   781          spin_unlock(&link->graph_obj.mdev->lock);

Thanks for pointing it!

Quoted 2 lines

Do we need this unlock any more?

Yes.

Quoted 1 line

Haven't we freed the lock on the previous line?

No. The lock is at the media_device struct, with is not freed here.

What we actually need is to cache mdev:

	struct media_device *mdev = link->graph_obj.mdev;

	spin_lock(&mdev->lock)
	__media_remove_intf_link(link);
	spin_unlock(&mdev->lock)

Probably the same thing is needed on other similar logic.

I guess gcc optimizer actually does the right thing, but we should fix it to remove the static analyzer warnings.

Regards,
Mauro

Quoted 5 lines
   782  }

regards,
dan carpenter

author
#2 lore raw
Quoted 2 lines · Mauro Carvalho Chehab

I guess gcc optimizer actually does the right thing, but we should fix it to remove the static analyzer warnings.

It probably crashes if you enable poisoning freed memory? (I haven't tested).

regards,
dan carpenter

#3 lore raw

Em Mon, 14 Dec 2015 23:04:34 +0300
Dan Carpenter <[email protected]> escreveu:

Quoted 6 lines · Mauro Carvalho Chehab
Quoted 2 lines

I guess gcc optimizer actually does the right thing, but we should fix it to remove the static analyzer warnings.

It probably crashes if you enable poisoning freed memory? (I haven't tested).

I tested with KASAN: when I made such patch: it worked fine. I suspect that gcc optimized the code to cache the value. Anyway, this should be fixed.

Thanks for pointing the issue!

Regards,
Mauro