[PATCH] Check fops_get() return value - Kernel

This is a discussion on [PATCH] Check fops_get() return value - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] Check fops_get() return value

  1. [PATCH] Check fops_get() return value

    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
    Signed-off-by: Laurent Pinchart
    ---
    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] Check fops_get() return value

    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
    > Signed-off-by: Laurent Pinchart
    > ---
    > 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


    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
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] Check fops_get() return value

    On Thu, 16 Oct 2008 15:28:00 +0200
    Takashi Iwai 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


    You have my ack also.

    Acked-by: Mauro Carvalho Chehab

    Cheers,
    Mauro
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread