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

[Max Kellermann]

On 2016/06/16 18:06, Shuah Khan  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shuah Khan]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Max Kellermann]

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 
---
 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(>refcount);
+}
+
+static void dvb_ca_private_put(struct dvb_ca_private *ca)
+{
+   kref_put(>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(>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;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html