* [PATCH] Check fops_get() return value
@ 2008-10-16 13:02 Laurent Pinchart
2008-10-16 13:28 ` Takashi Iwai
0 siblings, 1 reply; 3+ messages in thread
From: Laurent Pinchart @ 2008-10-16 13:02 UTC (permalink / raw)
To: linux-kernel; +Cc: airlied, mchehab, perex, tiwai, alan-jenkins
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 <alan-jenkins@tuffmail.co.uk>
Signed-off-by: Laurent Pinchart <laurent.pinchart@skynet.be>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Check fops_get() return value
2008-10-16 13:02 [PATCH] Check fops_get() return value Laurent Pinchart
@ 2008-10-16 13:28 ` Takashi Iwai
2008-10-24 14:33 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2008-10-16 13:28 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-kernel, airlied, mchehab, perex, alan-jenkins
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 <alan-jenkins@tuffmail.co.uk>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@skynet.be>
> ---
> 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 <tiwai@suse.de>
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Check fops_get() return value
2008-10-16 13:28 ` Takashi Iwai
@ 2008-10-24 14:33 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 3+ messages in thread
From: Mauro Carvalho Chehab @ 2008-10-24 14:33 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Laurent Pinchart, linux-kernel, airlied, perex, alan-jenkins
On Thu, 16 Oct 2008 15:28:00 +0200
Takashi Iwai <tiwai@suse.de> 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 <tiwai@suse.de>
You have my ack also.
Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Cheers,
Mauro
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-10-24 14:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-16 13:02 [PATCH] Check fops_get() return value Laurent Pinchart
2008-10-16 13:28 ` Takashi Iwai
2008-10-24 14:33 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).