Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423408 > unrolled thread

[PATCH 1/3] drivers/media/dvb-core/en50221: use kref to manage struct dvb_ca_private

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] drivers/media/dvb-core/en50221: use kref to manage  struct dvb_ca_private Max Kellermann <max@duempel.org> - 2016-06-15 22:30 +0200
    [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Max Kellermann <max@duempel.org> - 2016-06-15 22:30 +0200
      Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Shuah Khan <shuahkh@osg.samsung.com> - 2016-06-16 18:30 +0200
        Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Max Kellermann <max@duempel.org> - 2016-06-16 20:50 +0200
          Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Shuah Khan <shuahkh@osg.samsung.com> - 2016-06-16 21:00 +0200
      Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Sakari Ailus <sakari.ailus@iki.fi> - 2016-06-17 15:00 +0200
        Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in  _destroy() Max Kellermann <max@duempel.org> - 2016-06-17 15:10 +0200
    Re: [PATCH 1/3] drivers/media/dvb-core/en50221: use kref to manage  struct dvb_ca_private Shuah Khan <shuahkh@osg.samsung.com> - 2016-06-16 18:10 +0200
      Re: [PATCH 1/3] drivers/media/dvb-core/en50221: use kref to manage  struct dvb_ca_private Max Kellermann <max@duempel.org> - 2016-06-16 20:40 +0200

#1423408 — [PATCH 1/3] drivers/media/dvb-core/en50221: use kref to manage struct dvb_ca_private

Don't free the object until the file handle has been closed.  Fixes
use-after-free bug which occurs when I disconnect my DVB-S received
while VDR is running.

Signed-off-by: Max Kellermann <max@duempel.org>
---
 drivers/media/dvb-core/dvb_ca_en50221.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c
index b1e3a26..b5b5b19 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -123,6 +123,7 @@ struct dvb_ca_slot {
 
 /* Private CA-interface information */
 struct dvb_ca_private {
+	struct kref refcount;
 
 	/* pointer back to the public data structure */
 	struct dvb_ca_en50221 *pub;
@@ -173,6 +174,22 @@ static void dvb_ca_private_free(struct dvb_ca_private *ca)
 	kfree(ca);
 }
 
+static void dvb_ca_private_release(struct kref *ref)
+{
+	struct dvb_ca_private *ca = container_of(ref, struct dvb_ca_private, refcount);
+	dvb_ca_private_free(ca);
+}
+
+static void dvb_ca_private_get(struct dvb_ca_private *ca)
+{
+	kref_get(&ca->refcount);
+}
+
+static void dvb_ca_private_put(struct dvb_ca_private *ca)
+{
+	kref_put(&ca->refcount, dvb_ca_private_release);
+}
+
 static void dvb_ca_en50221_thread_wakeup(struct dvb_ca_private *ca);
 static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * ebuf, int ecount);
 static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * ebuf, int ecount);
@@ -1570,6 +1587,8 @@ static int dvb_ca_en50221_io_open(struct inode *inode, struct file *file)
 	dvb_ca_en50221_thread_update_delay(ca);
 	dvb_ca_en50221_thread_wakeup(ca);
 
+	dvb_ca_private_get(ca);
+
 	return 0;
 }
 
@@ -1598,6 +1617,8 @@ static int dvb_ca_en50221_io_release(struct inode *inode, struct file *file)
 
 	module_put(ca->pub->owner);
 
+	dvb_ca_private_put(ca);
+
 	return err;
 }
 
@@ -1693,6 +1714,7 @@ int dvb_ca_en50221_init(struct dvb_adapter *dvb_adapter,
 		ret = -ENOMEM;
 		goto exit;
 	}
+	kref_init(&ca->refcount);
 	ca->pub = pubca;
 	ca->flags = flags;
 	ca->slot_count = slot_count;
@@ -1772,6 +1794,6 @@ void dvb_ca_en50221_release(struct dvb_ca_en50221 *pubca)
 	for (i = 0; i < ca->slot_count; i++) {
 		dvb_ca_en50221_slot_shutdown(ca, i);
 	}
-	dvb_ca_private_free(ca);
+	dvb_ca_private_put(ca);
 	pubca->private = NULL;
 }

[toc] | [next] | [standalone]

#1423417 — [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

media_gobj_destroy() may be called twice on one instance - once by
media_device_unregister() and again by dvb_media_device_free().  The
function media_remove_intf_links() establishes and documents the
convention that mdev==NULL means that the object is not registered,
but nobody ever NULLs this variable.  So this patch really implements
this behavior, and adds another mdev==NULL check to
media_gobj_destroy() to protect against double removal.

Signed-off-by: Max Kellermann <max@duempel.org>
---
 drivers/media/media-entity.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index d8a2299..9526338 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -203,10 +203,16 @@ void media_gobj_destroy(struct media_gobj *gobj)
 {
 	dev_dbg_obj(__func__, gobj);
 
+	/* Do nothing if the object is not linked. */
+	if (gobj->mdev == NULL)
+		return;
+
 	gobj->mdev->topology_version++;
 
 	/* Remove the object from mdev list */
 	list_del(&gobj->list);
+
+	gobj->mdev = NULL;
 }
 
 int media_entity_pads_init(struct media_entity *entity, u16 num_pads,

[toc] | [prev] | [next] | [standalone]

#1424246 — Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

On 06/15/2016 02:15 PM, Max Kellermann wrote:
> media_gobj_destroy() may be called twice on one instance - once by
> media_device_unregister() and again by dvb_media_device_free().  The
> function media_remove_intf_links() establishes and documents the
> convention that mdev==NULL means that the object is not registered,
> but nobody ever NULLs this variable.  So this patch really implements
> this behavior, and adds another mdev==NULL check to
> media_gobj_destroy() to protect against double removal.

Are you seeing null pointer dereference on gobj->mdev? In any case,
we have to look at if there is a missing mutex hold that creates a
race between media_device_unregister() and dvb_media_device_free()

I don't this patch will solve the race condition.

thanks,
-- Shuah


> 
> Signed-off-by: Max Kellermann <max@duempel.org>
> ---
>  drivers/media/media-entity.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index d8a2299..9526338 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -203,10 +203,16 @@ void media_gobj_destroy(struct media_gobj *gobj)
>  {
>  	dev_dbg_obj(__func__, gobj);
>  
> +	/* Do nothing if the object is not linked. */
> +	if (gobj->mdev == NULL)
> +		return;
> +
>  	gobj->mdev->topology_version++;
>  
>  	/* Remove the object from mdev list */
>  	list_del(&gobj->list);
> +
> +	gobj->mdev = NULL;
>  }
>  
>  int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1424344 — Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

On 2016/06/16 18:24, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> On 06/15/2016 02:15 PM, Max Kellermann wrote:
> > media_gobj_destroy() may be called twice on one instance - once by
> > media_device_unregister() and again by dvb_media_device_free().  The
> > function media_remove_intf_links() establishes and documents the
> > convention that mdev==NULL means that the object is not registered,
> > but nobody ever NULLs this variable.  So this patch really implements
> > this behavior, and adds another mdev==NULL check to
> > media_gobj_destroy() to protect against double removal.
> 
> Are you seeing null pointer dereference on gobj->mdev? In any case,
> we have to look at if there is a missing mutex hold that creates a
> race between media_device_unregister() and dvb_media_device_free()
> 
> I don't this patch will solve the race condition.

I think we misunderstand.  This is not about a race condition.  And
the problem cannot be a NULL pointer dereference.

That's because nobody NULLs the pointer!

Pointer NULLing is what my patch adds, and AFTER my patch, there may
be NULL pointer dereferences (if there are more previously existing
bugs, which we should fix as well).  I added this NULL assignment
because there are NULL checks - and if nobody NULLs the pointer, that
check doesn't make any sense!

Max

[toc] | [prev] | [next] | [standalone]

#1424351 — Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

On 06/16/2016 12:43 PM, Max Kellermann wrote:
> On 2016/06/16 18:24, Shuah Khan <shuahkh@osg.samsung.com> wrote:
>> On 06/15/2016 02:15 PM, Max Kellermann wrote:
>>> media_gobj_destroy() may be called twice on one instance - once by
>>> media_device_unregister() and again by dvb_media_device_free().  The
>>> function media_remove_intf_links() establishes and documents the
>>> convention that mdev==NULL means that the object is not registered,
>>> but nobody ever NULLs this variable.  So this patch really implements
>>> this behavior, and adds another mdev==NULL check to
>>> media_gobj_destroy() to protect against double removal.
>>
>> Are you seeing null pointer dereference on gobj->mdev? In any case,
>> we have to look at if there is a missing mutex hold that creates a
>> race between media_device_unregister() and dvb_media_device_free()
>>
>> I don't this patch will solve the race condition.
> 
> I think we misunderstand.  This is not about a race condition.  And
> the problem cannot be a NULL pointer dereference.
> 
> That's because nobody NULLs the pointer!

I see 7 calls to media_gobj_destroy(). In 6 cases, calling routines
fee the pointer that contains the graph_obj.

__media_device_unregister_entity() sets mdev ot null.

entity->graph_obj.mdev = NULL;

That is why I am confused when you say it never set to null.

thanks,
-- Shuah

[toc] | [prev] | [next] | [standalone]

#1425042 — Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

Hi Max,

On Wed, Jun 15, 2016 at 10:15:07PM +0200, Max Kellermann wrote:
> media_gobj_destroy() may be called twice on one instance - once by
> media_device_unregister() and again by dvb_media_device_free().  The

Is that something that should really happen, and why? The same object should
not be unregistered more than once --- in many call paths gobj
unregistration is followed by kfree() on the gobj.

> function media_remove_intf_links() establishes and documents the
> convention that mdev==NULL means that the object is not registered,
> but nobody ever NULLs this variable.  So this patch really implements
> this behavior, and adds another mdev==NULL check to
> media_gobj_destroy() to protect against double removal.
> 
> Signed-off-by: Max Kellermann <max@duempel.org>
> ---
>  drivers/media/media-entity.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index d8a2299..9526338 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -203,10 +203,16 @@ void media_gobj_destroy(struct media_gobj *gobj)
>  {
>  	dev_dbg_obj(__func__, gobj);
>  
> +	/* Do nothing if the object is not linked. */
> +	if (gobj->mdev == NULL)
> +		return;
> +
>  	gobj->mdev->topology_version++;
>  
>  	/* Remove the object from mdev list */
>  	list_del(&gobj->list);
> +
> +	gobj->mdev = NULL;
>  }
>  
>  int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1425057 — Re: [PATCH 2/3] drivers/media/media-entity: clear media_gobj.mdev in _destroy()

On 2016/06/17 14:53, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> On Wed, Jun 15, 2016 at 10:15:07PM +0200, Max Kellermann wrote:
> > media_gobj_destroy() may be called twice on one instance - once by
> > media_device_unregister() and again by dvb_media_device_free().  The
> 
> Is that something that should really happen, and why? The same object should
> not be unregistered more than once --- in many call paths gobj
> unregistration is followed by kfree() on the gobj.

True, it should not happen, and I think the code is currently
misdesigned (or I just don't grasp it correctly; I may be wrong).

The "gobj" is inserted into a linked list, the list's owner
(media_device) feels responsible to free items in that list.  Plus,
the dvb_device instances holds a pointer and also tries to free it.

Usually, dvbdev.c destruction gets called first, which removes the
"gobj" from the linked list, and media_device never sees it during its
own destruction.  But that ordering is all but guaranteed.  It just
happens to be that way under "normal" circumstances.

None of this makes any sense to me.  There appears to be lots of bogus
and unsafe code.  I'm still waiting for somebody with more clue to
enlighten me.

Max

[toc] | [prev] | [next] | [standalone]

#1424225

On 06/15/2016 02:15 PM, Max Kellermann wrote:
> Don't free the object until the file handle has been closed.  Fixes
> use-after-free bug which occurs when I disconnect my DVB-S received
> while VDR is running.

Which file handle? /dev/dvb---

There seems to be a problem in the driver release routine:
dvb_ca_en50221_release() routine:

	kfree(ca->slot_info);
	dvb_unregister_device(ca->dvbdev);
	kfree(ca);

I think this should be since ioctl references slot info

	dvb_unregister_device(ca->dvbdev);
	kfree(ca->slot_info);
	kfree(ca);

I think dvb_ca_en50221_release() and dvb_ca_en50221_io_do_ioctl()
should serialize access to ca. dvb_ca_en50221_io_do_ioctl() holds
the ioctl_mutex, however, dvb_ca_en50221_release() could happen while
ioctl is in progress. Maybe you can try fixing those first.

As I mentioned in my review on your 3/3 patch, adding a kref here
adds more refcounted objects to the mix. You want to avoid that.

thanks,
-- Shuah

[toc] | [prev] | [next] | [standalone]

#1424342

On 2016/06/16 18:06, Shuah Khan <shuahkh@osg.samsung.com> wrote:
> On 06/15/2016 02:15 PM, Max Kellermann wrote:
> > Don't free the object until the file handle has been closed.  Fixes
> > use-after-free bug which occurs when I disconnect my DVB-S received
> > while VDR is running.
> 
> Which file handle? /dev/dvb---

I don't know which one triggers it.  I get crashes with VDR, and VDR
opens all of them (ca0, demux0, frontend0), but won't release the file
handles even if they become defunct.  Only restarting the VDR process
leads to recovery (or crash).

> I think dvb_ca_en50221_release() and dvb_ca_en50221_io_do_ioctl()
> should serialize access to ca. dvb_ca_en50221_io_do_ioctl() holds
> the ioctl_mutex, however, dvb_ca_en50221_release() could happen while
> ioctl is in progress. Maybe you can try fixing those first.

True, there are LOTS of race conditions in the DVB code.  I see them
everywhere.  But that's orthogonal to my patch, isn't it?

> As I mentioned in my review on your 3/3 patch, adding a kref here
> adds more refcounted objects to the mix. You want to avoid that.

Mauro asked me to add the kref.  What is your suggestion to fix the
use-after-free bug?

I have a problem here, as mentioned in my last email: I don't know how
all of this is supposed to be, how it was designed; all I see is bugs
inside strange code, and I have to guess the previous author's
intentions and try to do the best to fix the code.

Max

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web