Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Shuah Khan]

On 04/28/2016 01:19 AM, Lars-Peter Clausen wrote:
> On 04/27/2016 11:56 PM, Shuah Khan wrote:
dev_dbg(mdev->dev, "Media device unregistered\n");
  }
 diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
 index 29409f4..9af9ba1 100644
 --- a/drivers/media/media-devnode.c
 +++ b/drivers/media/media-devnode.c
 @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file 
 *filp)
mutex_unlock(_devnode_lock);
return -ENXIO;
}
 +
 +  kobject_get(>kobj);
>>>
>>> This is not necessary, and if it was it would be prone to race condition as
>>> the last reference could be dropped before this line. But assigning the cdev
>>> parent makes sure that we always have a reference to the object while the
>>> open() callback is running.
>>
>> I don't see cdev parent kobj get in cdev_get() which does kobject_get()
>> on cdev->kobj. Is that enough to get the reference?
>>
>> cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is
>> the reason why I added a get here and put in media_release().
>>
> 
> The cdev takes the parent reference when created and only drops it once it
> is released. So as long as the cdev exists there is a reference to the
> parent. While cdev_del() puts one reference to the cdev there is also one
> reference for each open file. So as long as there is a open file there is a
> reference to the parent as well.
> 
>> I can remove the get and put and test. Looks like I am not checking
>> kobject_get() return value which isn't good?
> 
> kobject_get() can't fail.

Yes looks that way, yet there are so many places in lib/kobject.c
that check for kobject_get() returning NULL. :)

> 
>>
>>>
 +
/* and increase the device refcount */
get_device(>dev);
mutex_unlock(_devnode_lock);
  /*
>>> [...]
 diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
 index fe42f08..ba4bdaa 100644
 --- a/include/media/media-devnode.h
 +++ b/include/media/media-devnode.h
 @@ -70,7 +70,9 @@ struct media_file_operations {
   * @fops: pointer to struct _file_operations with media device ops
   * @dev:  struct device pointer for the media controller device
   * @cdev: struct cdev pointer character device
 + * @kobj: struct kobject
   * @parent:   parent device
 + * @media_dev:media device
   * @minor:device node minor number
   * @flags:flags, combination of the MEDIA_FLAG_* constants
   * @release:  release callback called at the end of 
 media_devnode_release()
 @@ -87,7 +89,9 @@ struct media_devnode {
/* sysfs */
struct device dev;  /* media device */
struct cdev cdev;   /* character device */
 +  struct kobject kobj;/* set as cdev parent kobj */
>>>
>>> You don't need a extra kobj. Just use the struct dev kobj.
>>
>> Yeah I can use that as long as I can override the default release
>> function with media_devnode_free(). media_devnode should stick around
>> until the last app closes /dev/mediaX even if the media_device is no
>> longer registered. i.e media_ioctl should be able to check if devnode
>> is registered or not. I think I am missing something and don't understand
>> how struct dev kobj can be used.
> 
> The struct dev that is embedded into th media_devnode as the same live time
> as the media_devnode itself. In addition to that struct device is a
> reference counted object. This means a structure that embeds struct device
> must not be freed until the last reference is dropped.
> 
> What you do here is introduce a independent reference counting mechanism for
> the same structure. Which means if there is a reference to struct device,
> but not to the new kobj you end up with a use-after-free again.
> 
> The solution is to only use one reference counting mechanism (the struct
> device) and intialize the cdev kobj parent to the device kobj and whenever
> you did kobj_{get,put}() replace that with {get,put}_device(). And in the
> device release callback free the struct media_devnode.
> 

Okay. It is all well and good that I can use the kobj in devnode->dev,
however, devnode->dev doesn't get initialized in device_register(>dev)
and cdev_add() is the one that does kobject_get() on the cdev parent kobj.

I am seeing:
[   45.724866] au0828: Registered device AU0828 [Hauppauge HVR950Q]
[   45.724961] [ cut here ]
[   45.724975] WARNING: CPU: 1 PID: 312 at lib/kobject.c:597 
kobject_get+0x8f/0xf0
[   45.724982] kobject: '(null)' (8801f6166530): is not initialized, yet 
kobject_get() is being called.

warnings as soon as drivers do cdev_add().

This will be a problem at cdev_del() time, since cdev_del() is done after
device_unregister(>dev) and device_del() deletes the dev->kobj

In other words, devnode->dev lifetime is going to be within
device_register(>dev) and 

Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Mauro Carvalho Chehab]

Em Wed, 27 Apr 2016 07:51:08 -0600
Shuah Khan  escreveu:

> > - cdev patch;
> > - kref patch.
> > 
> > As a bonus side, by breaking into that, it helps to identify what
> > fixes are needed if we found similar issues at the other parts of
> > the subsystems.  
> 
> No problem breaking the it into 3 patches. I think the order should
> be kref and the a patch to set cdev kobj parent. Is that what you
> had in mind?

Works for me.

> 
> > 
> > If I remember well, I ended by having some cdev troubles with the
> > V4L2 core on one of my stress test. So, this is something that
> > we want to double check at RC, DVB and V4L parts that handle
> > cdev, and eventually porting the changes to the core of those
> > subsystems.  
> 
> Is that when you were playing with allocating cdev as opposed to
> setting parent. btw. just setting parent isn't enough. Kobject
> is necessary as it can then invoke the kobject put handler from
> cdev-core.

V4L2 has kref, as far as I remember, but not sure if it is doing
the right thing. As I pointed on the previous e-mail, I'm getting
some cdev issues that seem to be related to V4L2, like this:

[ 1002.856150] general protection fault:  [#5] SMP KASAN
[ 1002.859995] Modules linked in: ir_lirc_codec lirc_dev au8522_dig xc5000 
tuner au8522_decoder au8522_common au0828 videobuf2_vmalloc videobuf2_memops 
videobuf2_v4l2 videobuf2_core tveeprom dvb_core rc_core v4l2_common videodev 
media cpufreq_powersave cpufreq_conservative cpufreq_userspace cpufreq_stats 
parport_pc ppdev lp parport snd_hda_codec_hdmi intel_rapl x86_pkg_temp_thermal 
intel_powerclamp coretemp kvm_intel kvm iTCO_wdt iTCO_vendor_support irqbypass 
crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel i915 
snd_hda_codec_realtek snd_hda_codec_generic sha256_ssse3 sha256_generic hmac 
drbg btusb btrtl btbcm aesni_intel evdev snd_hda_intel aes_x86_64 lrw btintel 
i2c_algo_bit snd_hda_codec gf128mul bluetooth glue_helper drm_kms_helper 
snd_hwdep ablk_helper cryptd snd_hda_core drm serio_raw
[ 1002.870406]  rfkill sg snd_pcm pcspkr snd_timer mei_me snd mei lpc_ich 
i2c_i801 mfd_core soundcore battery dw_dmac video dw_dmac_core 
i2c_designware_platform i2c_designware_core acpi_pad button tpm_tis tpm ext4 
crc16 jbd2 mbcache dm_mod sd_mod ahci libahci psmouse libata e1000e ehci_pci 
xhci_pci ptp scsi_mod ehci_hcd pps_core xhci_hcd fan thermal sdhci_acpi sdhci 
mmc_core i2c_hid hid
[ 1002.874971] CPU: 2 PID: 3640 Comm: v4l_id Tainted: G  D 
4.6.0-rc2+ #68
[ 1002.877246] Hardware name:  /NUC5i7RYB, BIOS 
RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
[ 1002.879650] task: 88009c273000 ti: 880035c7 task.ti: 
880035c7
[ 1002.882891] RIP: 0010:[]  [] 
usb_ifnum_to_if+0x31/0x270
[ 1002.886337] RSP: 0018:880035c778d0  EFLAGS: 00010282
[ 1002.889950] RAX: dc00 RBX:  RCX: 11001364fc5e
[ 1002.892009] RDX: 009e RSI:  RDI: 04f0
[ 1002.893996] RBP: 880035c77908 R08: 0001 R09: 0001
[ 1002.895895] R10: 8803a239abf0 R11:  R12: a12be0c0
[ 1002.898465] R13: 88009b27eb3c R14: 88009b27c080 R15: a12a89b0
[ 1002.902048] FS:  7f0312a7e700() GS:8803c690() 
knlGS:
[ 1002.905584] CS:  0010 DS:  ES:  CR0: 80050033
[ 1002.908676] CR2: 01c84db0 CR3: 0003a1cd8000 CR4: 003406e0
[ 1002.910528] Stack:
[ 1002.912378]  8803bf048000 88009b27e2f0 88009b27c000 
a12be0c0
[ 1002.915578]  88009b27eb3c 88009b27c080 a12a89b0 
880035c77940
[ 1002.919242]  a12a4a45 880035c77940 88009b27c000 

[ 1002.921590] Call Trace:
[ 1002.923451]  [] ? au0828_v4l2_close+0x5a0/0x5a0 [au0828]
[ 1002.925339]  [] au0828_analog_stream_enable+0x85/0x330 
[au0828]
[ 1002.927234]  [] au0828_v4l2_open+0x161/0x350 [au0828]
[ 1002.930247]  [] ? au0828_v4l2_close+0x5a0/0x5a0 [au0828]
[ 1002.932695]  [] v4l2_open+0x1d1/0x350 [videodev]
[ 1002.934556]  [] chrdev_open+0x1f1/0x580
[ 1002.936446]  [] ? cdev_put+0x50/0x50
[ 1002.938301]  [] do_dentry_open+0x5d7/0xac0
[ 1002.941460]  [] ? cdev_put+0x50/0x50
[ 1002.943705]  [] vfs_open+0x16b/0x1e0
[ 1002.945529]  [] ? may_open+0x14b/0x260
[ 1002.947334]  [] path_openat+0x4f7/0x3a00
[ 1002.949259]  [] ? alloc_debug_processing+0x75/0x1c0
[ 1002.951185]  [] ? vfs_create+0x390/0x390
[ 1002.953004]  [] ? __kernel_text_address+0x7e/0xa0
[ 1002.954820]  [] ? print_context_stack+0x7f/0xf0
[ 1002.956646]  [] ? debug_check_no_locks_freed+0x290/0x290
[ 1002.959007]  [] ? create_object+0x3eb/0x940
[ 1002.962537]  [] ? trace_hardirqs_on_caller+0x16/0x590
[ 1002.966090]  [] do_filp_open+0x175/0x230
[ 1002.969603]  [] ? user_path_mountpoint_at+0x40/0x40
[ 1002.973088]  [] ? _raw_spin_unlock+0x27/0x40
[ 1002.974921]  [] ? __alloc_fd+0x19a/0x4b0
[ 1002.976759]  [] ? kmem_cache_alloc+0x233/0x300
[ 

Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Mauro Carvalho Chehab]

Em Wed, 27 Apr 2016 15:56:33 -0600
Shuah Khan  escreveu:

> On 04/27/2016 10:43 AM, Lars-Peter Clausen wrote:
> > Looks mostly good, a few comments.
> > 
> > On 04/27/2016 05:08 AM, Shuah Khan wrote:
> > [...]  
> >> @@ -428,7 +428,7 @@ static long media_device_ioctl(struct file *filp, 
> >> unsigned int cmd,
> >>   unsigned long arg)
> >>  {
> >>struct media_devnode *devnode = media_devnode_data(filp);
> >> -  struct media_device *dev = to_media_device(devnode);  
> > 
> > Can we keep the helper macro, means we don't need to touch this code.  
> 
> Yeah. I have been thinking about that as well. It avoids changes
> and abstracts it.

I don't like the idea of keeping the macro. It is used only on
two cases:

1) inside the core;
2) on two drivers that check if the media device is registered.

On the first case, if something changes, we want to be aware
about that. On the second case, IMHO, the best would be to have
a macro that would take struct media_device as argument, keeping
media_devnode hidden from drivers. 

> 
> >   
> >> +  struct media_device *dev = devnode->media_dev;  
> > 
> > You need a lock to protect this from running concurrently with
> > media_device_unregister() otherwise the struct might be freed while still in
> > use.
> >   
> 
> Right. This needs to be protected.
> 
> >>long ret;
> >>  
> >>switch (cmd) {  
> > [...]  
> >> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct 
> >> media_device *mdev,
> >>  {
> >>int ret;
> >>  
> >> +  mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL);  
> > 
> > sizeof(*mdev->devnode) is preferred kernel style,  
> 
> Yeah. Force of habit, I keep forgetting it.
> 
> >   
> >> +  if (!mdev->devnode)
> >> +  return -ENOMEM;
> >> +
> >>/* Register the device node. */
> >> -  mdev->devnode.fops = _device_fops;
> >> -  mdev->devnode.parent = mdev->dev;
> >> -  mdev->devnode.release = media_device_release;
> >> +  mdev->devnode->fops = _device_fops;
> >> +  mdev->devnode->parent = mdev->dev;
> >> +  mdev->devnode->media_dev = mdev;
> >> +  mdev->devnode->release = media_device_release;  
> > 
> > This should no longer be necessary. Just drop the release callback 
> > altogether.  
> 
> It does nothing at the moment. I believe the intent is for this routine
> to invoke any driver hooks if any at media_device level. It gets called
> from media_devnode_release() which is the media_devnode->dev.release.
> I will look into if it can be removed.

Right now, media_device_release callback is not used, except
to print that the device got removed, if debug enabled. Yet,
this is a separate change. Better to send such as a separate
patch.

> 
> >   
> >>  
> >>/* Set version 0 to indicate user-space that the graph is static */
> >>mdev->topology_version = 0;
> >>
> > [...]  
> >> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device 
> >> *mdev)
> >>  
> >>spin_unlock(>lock);
> >>  
> >> -  device_remove_file(>devnode.dev, _attr_model);
> >> -  media_devnode_unregister(>devnode);
> >> +  device_remove_file(>devnode->dev, _attr_model);
> >> +  media_devnode_unregister(mdev->devnode);
> >> +  /* kfree devnode is done via kobject_put() handler */
> >> +  mdev->devnode = NULL;  
> > 
> > mdev->devnode->media_dev needs to be set to NULL.  
> 
> Yes. Thanks for catching it.
> 
> >   
> >>  
> >>dev_dbg(mdev->dev, "Media device unregistered\n");
> >>  }
> >> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> >> index 29409f4..9af9ba1 100644
> >> --- a/drivers/media/media-devnode.c
> >> +++ b/drivers/media/media-devnode.c
> >> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file 
> >> *filp)
> >>mutex_unlock(_devnode_lock);
> >>return -ENXIO;
> >>}
> >> +
> >> +  kobject_get(>kobj);  
> > 
> > This is not necessary, and if it was it would be prone to race condition as
> > the last reference could be dropped before this line. But assigning the cdev
> > parent makes sure that we always have a reference to the object while the
> > open() callback is running.  
> 
> I don't see cdev parent kobj get in cdev_get() which does kobject_get()
> on cdev->kobj. Is that enough to get the reference?
> 
> cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is
> the reason why I added a get here and put in media_release().
> 
> I can remove the get and put and test. Looks like I am not checking
> kobject_get() return value which isn't good?
> 
> >   
> >> +
> >>/* and increase the device refcount */
> >>get_device(>dev);
> >>mutex_unlock(_devnode_lock);
> >>  /*  
> > [...]  
> >> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> >> index fe42f08..ba4bdaa 100644
> >> --- a/include/media/media-devnode.h
> >> +++ b/include/media/media-devnode.h
> >> @@ -70,7 +70,9 @@ struct media_file_operations {
> >>   * @fops: 

Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Lars-Peter Clausen]

On 04/27/2016 11:56 PM, Shuah Khan wrote:
>>> dev_dbg(mdev->dev, "Media device unregistered\n");
>>>  }
>>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>>> index 29409f4..9af9ba1 100644
>>> --- a/drivers/media/media-devnode.c
>>> +++ b/drivers/media/media-devnode.c
>>> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file 
>>> *filp)
>>> mutex_unlock(_devnode_lock);
>>> return -ENXIO;
>>> }
>>> +
>>> +   kobject_get(>kobj);
>>
>> This is not necessary, and if it was it would be prone to race condition as
>> the last reference could be dropped before this line. But assigning the cdev
>> parent makes sure that we always have a reference to the object while the
>> open() callback is running.
> 
> I don't see cdev parent kobj get in cdev_get() which does kobject_get()
> on cdev->kobj. Is that enough to get the reference?
> 
> cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is
> the reason why I added a get here and put in media_release().
> 

The cdev takes the parent reference when created and only drops it once it
is released. So as long as the cdev exists there is a reference to the
parent. While cdev_del() puts one reference to the cdev there is also one
reference for each open file. So as long as there is a open file there is a
reference to the parent as well.

> I can remove the get and put and test. Looks like I am not checking
> kobject_get() return value which isn't good?

kobject_get() can't fail.

> 
>>
>>> +
>>> /* and increase the device refcount */
>>> get_device(>dev);
>>> mutex_unlock(_devnode_lock);
>>>  /*
>> [...]
>>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>>> index fe42f08..ba4bdaa 100644
>>> --- a/include/media/media-devnode.h
>>> +++ b/include/media/media-devnode.h
>>> @@ -70,7 +70,9 @@ struct media_file_operations {
>>>   * @fops:  pointer to struct _file_operations with media device ops
>>>   * @dev:   struct device pointer for the media controller device
>>>   * @cdev:  struct cdev pointer character device
>>> + * @kobj:  struct kobject
>>>   * @parent:parent device
>>> + * @media_dev: media device
>>>   * @minor: device node minor number
>>>   * @flags: flags, combination of the MEDIA_FLAG_* constants
>>>   * @release:   release callback called at the end of 
>>> media_devnode_release()
>>> @@ -87,7 +89,9 @@ struct media_devnode {
>>> /* sysfs */
>>> struct device dev;  /* media device */
>>> struct cdev cdev;   /* character device */
>>> +   struct kobject kobj;/* set as cdev parent kobj */
>>
>> You don't need a extra kobj. Just use the struct dev kobj.
> 
> Yeah I can use that as long as I can override the default release
> function with media_devnode_free(). media_devnode should stick around
> until the last app closes /dev/mediaX even if the media_device is no
> longer registered. i.e media_ioctl should be able to check if devnode
> is registered or not. I think I am missing something and don't understand
> how struct dev kobj can be used.

The struct dev that is embedded into th media_devnode as the same live time
as the media_devnode itself. In addition to that struct device is a
reference counted object. This means a structure that embeds struct device
must not be freed until the last reference is dropped.

What you do here is introduce a independent reference counting mechanism for
the same structure. Which means if there is a reference to struct device,
but not to the new kobj you end up with a use-after-free again.

The solution is to only use one reference counting mechanism (the struct
device) and intialize the cdev kobj parent to the device kobj and whenever
you did kobj_{get,put}() replace that with {get,put}_device(). And in the
device release callback free the struct media_devnode.
--
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] media: fix media_ioctl use-after-free when driver unbinds

[Shuah Khan]

On 04/27/2016 10:43 AM, Lars-Peter Clausen wrote:
> Looks mostly good, a few comments.
> 
> On 04/27/2016 05:08 AM, Shuah Khan wrote:
> [...]
>> @@ -428,7 +428,7 @@ static long media_device_ioctl(struct file *filp, 
>> unsigned int cmd,
>> unsigned long arg)
>>  {
>>  struct media_devnode *devnode = media_devnode_data(filp);
>> -struct media_device *dev = to_media_device(devnode);
> 
> Can we keep the helper macro, means we don't need to touch this code.

Yeah. I have been thinking about that as well. It avoids changes
and abstracts it.

> 
>> +struct media_device *dev = devnode->media_dev;
> 
> You need a lock to protect this from running concurrently with
> media_device_unregister() otherwise the struct might be freed while still in
> use.
> 

Right. This needs to be protected.

>>  long ret;
>>  
>>  switch (cmd) {
> [...]
>> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct 
>> media_device *mdev,
>>  {
>>  int ret;
>>  
>> +mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL);
> 
> sizeof(*mdev->devnode) is preferred kernel style,

Yeah. Force of habit, I keep forgetting it.

> 
>> +if (!mdev->devnode)
>> +return -ENOMEM;
>> +
>>  /* Register the device node. */
>> -mdev->devnode.fops = _device_fops;
>> -mdev->devnode.parent = mdev->dev;
>> -mdev->devnode.release = media_device_release;
>> +mdev->devnode->fops = _device_fops;
>> +mdev->devnode->parent = mdev->dev;
>> +mdev->devnode->media_dev = mdev;
>> +mdev->devnode->release = media_device_release;
> 
> This should no longer be necessary. Just drop the release callback altogether.

It does nothing at the moment. I believe the intent is for this routine
to invoke any driver hooks if any at media_device level. It gets called
from media_devnode_release() which is the media_devnode->dev.release.
I will look into if it can be removed.

> 
>>  
>>  /* Set version 0 to indicate user-space that the graph is static */
>>  mdev->topology_version = 0;
>>  
> [...]
>> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev)
>>  
>>  spin_unlock(>lock);
>>  
>> -device_remove_file(>devnode.dev, _attr_model);
>> -media_devnode_unregister(>devnode);
>> +device_remove_file(>devnode->dev, _attr_model);
>> +media_devnode_unregister(mdev->devnode);
>> +/* kfree devnode is done via kobject_put() handler */
>> +mdev->devnode = NULL;
> 
> mdev->devnode->media_dev needs to be set to NULL.

Yes. Thanks for catching it.

> 
>>  
>>  dev_dbg(mdev->dev, "Media device unregistered\n");
>>  }
>> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
>> index 29409f4..9af9ba1 100644
>> --- a/drivers/media/media-devnode.c
>> +++ b/drivers/media/media-devnode.c
>> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file 
>> *filp)
>>  mutex_unlock(_devnode_lock);
>>  return -ENXIO;
>>  }
>> +
>> +kobject_get(>kobj);
> 
> This is not necessary, and if it was it would be prone to race condition as
> the last reference could be dropped before this line. But assigning the cdev
> parent makes sure that we always have a reference to the object while the
> open() callback is running.

I don't see cdev parent kobj get in cdev_get() which does kobject_get()
on cdev->kobj. Is that enough to get the reference?

cdev_add() gets the cdev parent kobj and cdev_del() puts it back. That is
the reason why I added a get here and put in media_release().

I can remove the get and put and test. Looks like I am not checking
kobject_get() return value which isn't good?

> 
>> +
>>  /* and increase the device refcount */
>>  get_device(>dev);
>>  mutex_unlock(_devnode_lock);
>>  /*
> [...]
>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>> index fe42f08..ba4bdaa 100644
>> --- a/include/media/media-devnode.h
>> +++ b/include/media/media-devnode.h
>> @@ -70,7 +70,9 @@ struct media_file_operations {
>>   * @fops:   pointer to struct _file_operations with media device ops
>>   * @dev:struct device pointer for the media controller device
>>   * @cdev:   struct cdev pointer character device
>> + * @kobj:   struct kobject
>>   * @parent: parent device
>> + * @media_dev:  media device
>>   * @minor:  device node minor number
>>   * @flags:  flags, combination of the MEDIA_FLAG_* constants
>>   * @release:release callback called at the end of 
>> media_devnode_release()
>> @@ -87,7 +89,9 @@ struct media_devnode {
>>  /* sysfs */
>>  struct device dev;  /* media device */
>>  struct cdev cdev;   /* character device */
>> +struct kobject kobj;/* set as cdev parent kobj */
> 
> You don't need a extra kobj. Just use the struct dev kobj.

Yeah I can use that as long as I can override the default release
function with media_devnode_free(). 

Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Lars-Peter Clausen]

Looks mostly good, a few comments.

On 04/27/2016 05:08 AM, Shuah Khan wrote:
[...]
> @@ -428,7 +428,7 @@ static long media_device_ioctl(struct file *filp, 
> unsigned int cmd,
>  unsigned long arg)
>  {
>   struct media_devnode *devnode = media_devnode_data(filp);
> - struct media_device *dev = to_media_device(devnode);

Can we keep the helper macro, means we don't need to touch this code.

> + struct media_device *dev = devnode->media_dev;

You need a lock to protect this from running concurrently with
media_device_unregister() otherwise the struct might be freed while still in
use.

>   long ret;
>  
>   switch (cmd) {
[...]
> @@ -725,21 +726,26 @@ int __must_check __media_device_register(struct 
> media_device *mdev,
>  {
>   int ret;
>  
> + mdev->devnode = kzalloc(sizeof(struct media_devnode), GFP_KERNEL);

sizeof(*mdev->devnode) is preferred kernel style,

> + if (!mdev->devnode)
> + return -ENOMEM;
> +
>   /* Register the device node. */
> - mdev->devnode.fops = _device_fops;
> - mdev->devnode.parent = mdev->dev;
> - mdev->devnode.release = media_device_release;
> + mdev->devnode->fops = _device_fops;
> + mdev->devnode->parent = mdev->dev;
> + mdev->devnode->media_dev = mdev;
> + mdev->devnode->release = media_device_release;

This should no longer be necessary. Just drop the release callback altogether.

>  
>   /* Set version 0 to indicate user-space that the graph is static */
>   mdev->topology_version = 0;
>  
[...]
> @@ -813,8 +819,10 @@ void media_device_unregister(struct media_device *mdev)
>  
>   spin_unlock(>lock);
>  
> - device_remove_file(>devnode.dev, _attr_model);
> - media_devnode_unregister(>devnode);
> + device_remove_file(>devnode->dev, _attr_model);
> + media_devnode_unregister(mdev->devnode);
> + /* kfree devnode is done via kobject_put() handler */
> + mdev->devnode = NULL;

mdev->devnode->media_dev needs to be set to NULL.

>  
>   dev_dbg(mdev->dev, "Media device unregistered\n");
>  }
> diff --git a/drivers/media/media-devnode.c b/drivers/media/media-devnode.c
> index 29409f4..9af9ba1 100644
> --- a/drivers/media/media-devnode.c
> +++ b/drivers/media/media-devnode.c
> @@ -171,6 +171,9 @@ static int media_open(struct inode *inode, struct file 
> *filp)
>   mutex_unlock(_devnode_lock);
>   return -ENXIO;
>   }
> +
> + kobject_get(>kobj);

This is not necessary, and if it was it would be prone to race condition as
the last reference could be dropped before this line. But assigning the cdev
parent makes sure that we always have a reference to the object while the
open() callback is running.

> +
>   /* and increase the device refcount */
>   get_device(>dev);
>   mutex_unlock(_devnode_lock);
>  /*
[...]
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index fe42f08..ba4bdaa 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -70,7 +70,9 @@ struct media_file_operations {
>   * @fops:pointer to struct _file_operations with media device ops
>   * @dev: struct device pointer for the media controller device
>   * @cdev:struct cdev pointer character device
> + * @kobj:struct kobject
>   * @parent:  parent device
> + * @media_dev:   media device
>   * @minor:   device node minor number
>   * @flags:   flags, combination of the MEDIA_FLAG_* constants
>   * @release: release callback called at the end of media_devnode_release()
> @@ -87,7 +89,9 @@ struct media_devnode {
>   /* sysfs */
>   struct device dev;  /* media device */
>   struct cdev cdev;   /* character device */
> + struct kobject kobj;/* set as cdev parent kobj */

You don't need a extra kobj. Just use the struct dev kobj.

>   struct device *parent;  /* device parent */
> + struct media_device *media_dev; /* media device for the devnode */
>  
>   /* device info */
>   int minor;

--
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] media: fix media_ioctl use-after-free when driver unbinds

[Shuah Khan]

Hi Mauro,

On 04/27/2016 03:55 AM, Mauro Carvalho Chehab wrote:
> Hi Shuah,
> 
> Good work! I have a few notes below.
> 
> Em Tue, 26 Apr 2016 21:08:32 -0600
> Shuah Khan  escreveu:
> 
>> When driver unbind is run while media_ioctl is in progress, media_ioctl()
>> fails with use-after-free. This first use-after-free is followed by more
>> user-after-free errors in media_release(), kobject_put(), and cdev_put()
>> as driver unbind continues. This problem is found on uvcvideo, em28xx, and
>> au0828 drivers and fix has been tested on all three.
>>
>> This fix allocates media devnode and manages its lifetime separate from the
>> struct media_device. Adds kobject to the media_devnode structure and this
>> kobject is set as the cdev parent kobject. This allows cdev_add() to hold
>> a reference to it and release the reference in cdev_del() ensuring that the
>> media_devnode is not deallocated as long as the application has the cdev
>> open.
>>
>> The first error is below:
>>
>> [  472.424302] 
>> ==
>> [  472.424333] BUG: KASAN: use-after-free in media_ioctl+0xf0/0x130 [media] 
>> at addr 880027b72330
>> [  472.424341] Read of size 8 by task media_device_te/1794
>> [  472.424348] 
>> =
>> [  472.424356] BUG kmalloc-4096 (Not tainted): kasan: bad access detected
>> [  472.424361] 
>> -
>> [  472.431973] CPU: 1 PID: 1794 Comm: media_device_te Tainted: GB
>>4.6.0-rc5 #2
>> [  472.431988] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 
>> 68TTU Ver. F.04 08/03/2012
>> [  472.431996]  ea9edc00 88009ddffc78 81aecac3 
>> 8801fa403200
>> [  472.432016]  880027b72260 88009ddffca8 815359b2 
>> 8801fa403200
>> [  472.432040]  ea9edc00 880027b72260 a0c9cc60 
>> 88009ddffcd0
>> [  472.432059] Call Trace:
>> [  472.432079]  [] dump_stack+0x67/0x94
>> [  472.432092]  [] print_trailer+0x112/0x1a0
>> [  472.432108]  [] object_err+0x34/0x40
>> [  472.432125]  [] kasan_report_error+0x224/0x530
>> [  472.432148]  [] ? 
>> __media_device_get_topology+0x1850/0x1850 [media]
>> [  472.432167]  [] __asan_report_load8_noabort+0x43/0x50
>> [  472.432190]  [] ? media_ioctl+0x120/0x130 [media]
>> [  472.432209]  [] media_ioctl+0x120/0x130 [media]
>> [  472.432229]  [] do_vfs_ioctl+0x184/0xe80
>> [  472.432243]  [] ? ioctl_preallocate+0x1a0/0x1a0
>> [  472.432256]  [] ? __hrtimer_init+0x170/0x170
>> [  472.432272]  [] ? do_nanosleep+0x161/0x480
>> [  472.432298]  [] ? sigprocmask+0x290/0x290
>> [  472.432323]  [] ? __fget_light+0x139/0x200
>> [  472.432358]  [] SyS_ioctl+0x79/0x90
>> [  472.432381]  [] entry_SYSCALL_64_fastpath+0x18/0xa8
> 
> This patch is almost identical to my patch, as you took the same
> approach as I did:
>   https://patchwork.linuxtv.org/patch/33577/
> 
> So, I compared both to identify the differences. What I noticed is
> that:
> 
> 1) your patch is setting cdev->parent; in my case, I fixed on a
>separate patch. 
> 
> IMHO, this should be on a separate patch, as cdev is a separate bug.
> 
> 2) On my patch, I also fixed the error conditions at 
>__media_device_register(): currently, it has a few issues, and,
>after the patch, if an error occurs, mdev->devnode should be
>set to NULL;
> 
> 3) you added a kref. I would prefer to see this on a separate patch
>too, as this is not related with moving from an embedded struct
>to a dynamically allocated one. One logical change per patch,
>please.
> 
> There's also the point that you're using my patch, but removing
> my credits. In this specific case, as a developer, I don't mind
> much about that, as it is just one patch and didn't take much
> of my time to produce. Yet, maintainers should not allow stripping
> off credits, as this could cause troubles later.

I don't do things like using another developer's work and strip
credits.

I didn't user your patch. I started from scratch to solve the problem.
There is a good reason for that.

For one thing, your patch came out when we were dealing with lots of
problems with having au0828 and snd-us-audio in the mix. I also wanted
to start from a clean slate and not use any code that was done while we
were debugging two driver problems. As such, there a flood of patches
from you at that time, and I didn't know which ones are applicable
for a single driver case, and which ones aren't. So I just went the
route of clean slate.

That said, I am fine with you want to not take my patch and use yours.

> 
> So, IMHO, the best would be to split this patch in 3 patches:
> 
> - my patch (fixing the context changes);

I will leave it up to you to fix the context changes if any.
I can apply you patch as is and then add the cdev and kref
patch.

> - cdev patch;
> - kref patch.
> 
> 

Re: [PATCH] media: fix media_ioctl use-after-free when driver unbinds

[Mauro Carvalho Chehab]

Hi Shuah,

Good work! I have a few notes below.

Em Tue, 26 Apr 2016 21:08:32 -0600
Shuah Khan  escreveu:

> When driver unbind is run while media_ioctl is in progress, media_ioctl()
> fails with use-after-free. This first use-after-free is followed by more
> user-after-free errors in media_release(), kobject_put(), and cdev_put()
> as driver unbind continues. This problem is found on uvcvideo, em28xx, and
> au0828 drivers and fix has been tested on all three.
> 
> This fix allocates media devnode and manages its lifetime separate from the
> struct media_device. Adds kobject to the media_devnode structure and this
> kobject is set as the cdev parent kobject. This allows cdev_add() to hold
> a reference to it and release the reference in cdev_del() ensuring that the
> media_devnode is not deallocated as long as the application has the cdev
> open.
> 
> The first error is below:
> 
> [  472.424302] 
> ==
> [  472.424333] BUG: KASAN: use-after-free in media_ioctl+0xf0/0x130 [media] 
> at addr 880027b72330
> [  472.424341] Read of size 8 by task media_device_te/1794
> [  472.424348] 
> =
> [  472.424356] BUG kmalloc-4096 (Not tainted): kasan: bad access detected
> [  472.424361] 
> -
> [  472.431973] CPU: 1 PID: 1794 Comm: media_device_te Tainted: GB 
>   4.6.0-rc5 #2
> [  472.431988] Hardware name: Hewlett-Packard HP ProBook 6475b/180F, BIOS 
> 68TTU Ver. F.04 08/03/2012
> [  472.431996]  ea9edc00 88009ddffc78 81aecac3 
> 8801fa403200
> [  472.432016]  880027b72260 88009ddffca8 815359b2 
> 8801fa403200
> [  472.432040]  ea9edc00 880027b72260 a0c9cc60 
> 88009ddffcd0
> [  472.432059] Call Trace:
> [  472.432079]  [] dump_stack+0x67/0x94
> [  472.432092]  [] print_trailer+0x112/0x1a0
> [  472.432108]  [] object_err+0x34/0x40
> [  472.432125]  [] kasan_report_error+0x224/0x530
> [  472.432148]  [] ? 
> __media_device_get_topology+0x1850/0x1850 [media]
> [  472.432167]  [] __asan_report_load8_noabort+0x43/0x50
> [  472.432190]  [] ? media_ioctl+0x120/0x130 [media]
> [  472.432209]  [] media_ioctl+0x120/0x130 [media]
> [  472.432229]  [] do_vfs_ioctl+0x184/0xe80
> [  472.432243]  [] ? ioctl_preallocate+0x1a0/0x1a0
> [  472.432256]  [] ? __hrtimer_init+0x170/0x170
> [  472.432272]  [] ? do_nanosleep+0x161/0x480
> [  472.432298]  [] ? sigprocmask+0x290/0x290
> [  472.432323]  [] ? __fget_light+0x139/0x200
> [  472.432358]  [] SyS_ioctl+0x79/0x90
> [  472.432381]  [] entry_SYSCALL_64_fastpath+0x18/0xa8

This patch is almost identical to my patch, as you took the same
approach as I did:
https://patchwork.linuxtv.org/patch/33577/

So, I compared both to identify the differences. What I noticed is
that:

1) your patch is setting cdev->parent; in my case, I fixed on a
   separate patch. 

IMHO, this should be on a separate patch, as cdev is a separate bug.

2) On my patch, I also fixed the error conditions at 
   __media_device_register(): currently, it has a few issues, and,
   after the patch, if an error occurs, mdev->devnode should be
   set to NULL;

3) you added a kref. I would prefer to see this on a separate patch
   too, as this is not related with moving from an embedded struct
   to a dynamically allocated one. One logical change per patch,
   please.

There's also the point that you're using my patch, but removing
my credits. In this specific case, as a developer, I don't mind
much about that, as it is just one patch and didn't take much
of my time to produce. Yet, maintainers should not allow stripping
off credits, as this could cause troubles later.

So, IMHO, the best would be to split this patch in 3 patches:

- my patch (fixing the context changes);
- cdev patch;
- kref patch.

As a bonus side, by breaking into that, it helps to identify what
fixes are needed if we found similar issues at the other parts of
the subsystems.

If I remember well, I ended by having some cdev troubles with the
V4L2 core on one of my stress test. So, this is something that
we want to double check at RC, DVB and V4L parts that handle
cdev, and eventually porting the changes to the core of those
subsystems.

PS.: I did just a code review. I intend to test this along the
week.

Regards,
Mauro


> 
> Signed-off-by: Shuah Khan 
> ---
>  drivers/media/media-device.c   | 32 
>  drivers/media/media-devnode.c  | 23 +++
>  drivers/media/usb/au0828/au0828-core.c |  4 ++--
>  drivers/media/usb/uvc/uvc_driver.c |  2 +-
>  include/media/media-device.h   |  7 ++-
>  include/media/media-devnode.h  |  8 +++-
>  6 files changed, 55 insertions(+), 21 deletions(-)
> 
> diff --git