Several subsystem open handlers dereference the fops_get() return value
without checking it for nullness. This opens a race condition between the
open handler and module unloading.
A module can be marked as being unloaded (MODULE_STATE_GOING) before its exit
function is called and gets the chance to unregister the driver. During that
window open handlers can still be called, and fops_get() will fail in
try_module_get() and return a NULL pointer.
This change checks the fops_get() return value and returns -ENODEV if NULL.
Reported-by: Alan Jenkins <[email protected]>
Signed-off-by: Laurent Pinchart <[email protected]>
---
drivers/gpu/drm/drm_fops.c | 4 ++++
drivers/media/dvb/dvb-core/dvbdev.c | 5 +++++
sound/core/sound.c | 4 ++++
3 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 851a53f..ef5c295 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -186,6 +186,10 @@ int drm_stub_open(struct inode *inode, struct file *filp)
old_fops = filp->f_op;
filp->f_op = fops_get(&dev->driver->fops);
+ if (filp->f_op == NULL) {
+ filp->f_op = old_fops;
+ goto out;
+ }
if (filp->f_op->open && (err = filp->f_op->open(inode, filp))) {
fops_put(filp->f_op);
filp->f_op = fops_get(old_fops);
diff --git a/drivers/media/dvb/dvb-core/dvbdev.c b/drivers/media/dvb/dvb-core/dvbdev.c
index e713277..fb4e241 100644
--- a/drivers/media/dvb/dvb-core/dvbdev.c
+++ b/drivers/media/dvb/dvb-core/dvbdev.c
@@ -85,6 +85,10 @@ static int dvb_device_open(struct inode *inode, struct file *file)
file->private_data = dvbdev;
old_fops = file->f_op;
file->f_op = fops_get(dvbdev->fops);
+ if (file->f_op == NULL) {
+ file->f_op = old_fops;
+ goto fail;
+ }
if(file->f_op->open)
err = file->f_op->open(inode,file);
if (err) {
@@ -95,6 +99,7 @@ static int dvb_device_open(struct inode *inode, struct file *file)
unlock_kernel();
return err;
}
+fail:
unlock_kernel();
return -ENODEV;
}
diff --git a/sound/core/sound.c b/sound/core/sound.c
index c0685e2..8659b84 100644
--- a/sound/core/sound.c
+++ b/sound/core/sound.c
@@ -152,6 +152,10 @@ static int __snd_open(struct inode *inode, struct file *file)
}
old_fops = file->f_op;
file->f_op = fops_get(mptr->f_ops);
+ if (file->f_op == NULL) {
+ file->f_op = old_fops;
+ return -ENODEV;
+ }
if (file->f_op->open)
err = file->f_op->open(inode, file);
if (err) {
--
1.5.0
At Thu, 16 Oct 2008 15:02:40 +0200,
Laurent Pinchart wrote:
>
> Several subsystem open handlers dereference the fops_get() return value
> without checking it for nullness. This opens a race condition between the
> open handler and module unloading.
>
> A module can be marked as being unloaded (MODULE_STATE_GOING) before its exit
> function is called and gets the chance to unregister the driver. During that
> window open handlers can still be called, and fops_get() will fail in
> try_module_get() and return a NULL pointer.
>
> This change checks the fops_get() return value and returns -ENODEV if NULL.
>
> Reported-by: Alan Jenkins <[email protected]>
> Signed-off-by: Laurent Pinchart <[email protected]>
> ---
> drivers/gpu/drm/drm_fops.c | 4 ++++
> drivers/media/dvb/dvb-core/dvbdev.c | 5 +++++
> sound/core/sound.c | 4 ++++
> 3 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 851a53f..ef5c295 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -186,6 +186,10 @@ int drm_stub_open(struct inode *inode, struct file *filp)
>
> old_fops = filp->f_op;
> filp->f_op = fops_get(&dev->driver->fops);
> + if (filp->f_op == NULL) {
I think the more standard style is
if (!file->fop) {
Except for that minor issue, it looks good to me.
In any case,
Acked-by: Takashi Iwai <[email protected]>
thanks,
Takashi
> + filp->f_op = old_fops;
> + goto out;
> + }
> if (filp->f_op->open && (err = filp->f_op->open(inode, filp))) {
> fops_put(filp->f_op);
> filp->f_op = fops_get(old_fops);
> diff --git a/drivers/media/dvb/dvb-core/dvbdev.c b/drivers/media/dvb/dvb-core/dvbdev.c
> index e713277..fb4e241 100644
> --- a/drivers/media/dvb/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb/dvb-core/dvbdev.c
> @@ -85,6 +85,10 @@ static int dvb_device_open(struct inode *inode, struct file *file)
> file->private_data = dvbdev;
> old_fops = file->f_op;
> file->f_op = fops_get(dvbdev->fops);
> + if (file->f_op == NULL) {
> + file->f_op = old_fops;
> + goto fail;
> + }
> if(file->f_op->open)
> err = file->f_op->open(inode,file);
> if (err) {
> @@ -95,6 +99,7 @@ static int dvb_device_open(struct inode *inode, struct file *file)
> unlock_kernel();
> return err;
> }
> +fail:
> unlock_kernel();
> return -ENODEV;
> }
> diff --git a/sound/core/sound.c b/sound/core/sound.c
> index c0685e2..8659b84 100644
> --- a/sound/core/sound.c
> +++ b/sound/core/sound.c
> @@ -152,6 +152,10 @@ static int __snd_open(struct inode *inode, struct file *file)
> }
> old_fops = file->f_op;
> file->f_op = fops_get(mptr->f_ops);
> + if (file->f_op == NULL) {
> + file->f_op = old_fops;
> + return -ENODEV;
> + }
> if (file->f_op->open)
> err = file->f_op->open(inode, file);
> if (err) {
> --
> 1.5.0
>
On Thu, 16 Oct 2008 15:28:00 +0200
Takashi Iwai <[email protected]> wrote:
> >
> > old_fops = filp->f_op;
> > filp->f_op = fops_get(&dev->driver->fops);
> > + if (filp->f_op == NULL) {
>
> I think the more standard style is
>
> if (!file->fop) {
Yes. This seems a good cleanup to the patch.
>
> Except for that minor issue, it looks good to me.
> In any case,
>
> Acked-by: Takashi Iwai <[email protected]>
You have my ack also.
Acked-by: Mauro Carvalho Chehab <[email protected]>
Cheers,
Mauro