| Message ID | 1422557288-3617-1-git-send-email-rickard_strandqvist@spectrumdigital.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said: > Fix a possible null pointer dereference, there is > otherwise a risk of a possible null pointer dereference. > > This was found using a static code analysis program called cppcheck > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > --- > drivers/staging/media/lirc/lirc_zilog.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > /* find our IR struct */ > struct IR *ir = filep->private_data; > > - if (ir == NULL) { > - dev_err(ir->l.dev, "close: no private_data attached to the file!\n"); Yes, the dev_err() call is an obvious thinko. However, I'm not sure whether removing it entirely is right either. If there *should* be a struct IR * passed there, maybe some other printk() should be issued, or even a WARN_ON(!ir), or something?
On Thu, Jan 29, 2015 at 05:12:40PM -0500, Valdis.Kletnieks@vt.edu wrote: > On Thu, 29 Jan 2015 19:48:08 +0100, Rickard Strandqvist said: > > Fix a possible null pointer dereference, there is > > otherwise a risk of a possible null pointer dereference. > > > > This was found using a static code analysis program called cppcheck > > > > Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> > > --- > > drivers/staging/media/lirc/lirc_zilog.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > /* find our IR struct */ > > struct IR *ir = filep->private_data; > > > > - if (ir == NULL) { > > - dev_err(ir->l.dev, "close: no private_data attached to the file!\n"); > > Yes, the dev_err() call is an obvious thinko. > > However, I'm not sure whether removing it entirely is right either. If > there *should* be a struct IR * passed there, maybe some other printk() > should be issued, or even a WARN_ON(!ir), or something? We set filep->private_data to non-NULL in open() so I don't think it can be NULL here. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said: > > > - if (ir == NULL) { > > > - dev_err(ir->l.dev, "close: no private_data attached to the file !\n"); > > > > Yes, the dev_err() call is an obvious thinko. > > > > However, I'm not sure whether removing it entirely is right either. If > > there *should* be a struct IR * passed there, maybe some other printk() > > should be issued, or even a WARN_ON(!ir), or something? > > We set filep->private_data to non-NULL in open() so I don't think it can > be NULL here. Then probably the *right* fix is to remove the *entire* if statement, as we can't end up doing the 'return -ENODEV'....
2015-01-30 14:09 GMT+01:00 <Valdis.Kletnieks@vt.edu>: > On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said: > >> > > - if (ir == NULL) { >> > > - dev_err(ir->l.dev, "close: no private_data attached to the file > !\n"); >> > >> > Yes, the dev_err() call is an obvious thinko. >> > >> > However, I'm not sure whether removing it entirely is right either. If >> > there *should* be a struct IR * passed there, maybe some other printk() >> > should be issued, or even a WARN_ON(!ir), or something? >> >> We set filep->private_data to non-NULL in open() so I don't think it can >> be NULL here. > > Then probably the *right* fix is to remove the *entire* if statement, as > we can't end up doing the 'return -ENODEV'.... Hi Ok, but think or know. Who knows? Do the remove if patch? Kind regards Rickard Strandqvist -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-01-30 at 08:09 -0500, Valdis.Kletnieks@vt.edu wrote: > On Fri, 30 Jan 2015 16:00:02 +0300, Dan Carpenter said: > > > > > - if (ir == NULL) { > > > > - dev_err(ir->l.dev, "close: no private_data attached to the file > !\n"); > > > commit be4aa8157c981a8bb9634b886bf1180f97205259 removed the dprintk(), which didn't depend on ir->l.dev, with this dev_err() call. That was the wrong thing to do. pr_info() is probably the right thing to use, if one doesn't have a struct device instance. > > > Yes, the dev_err() call is an obvious thinko. > > > > > > However, I'm not sure whether removing it entirely is right either. If > > > there *should* be a struct IR * passed there, maybe some other printk() > > > should be issued, or even a WARN_ON(!ir), or something? > > > > We set filep->private_data to non-NULL in open() so I don't think it can > > be NULL here. > > Then probably the *right* fix is to remove the *entire* if statement, as > we can't end up doing the 'return -ENODEV'.... The if() clause is here as an artifact of being part of a mass port of lirc drivers from userspace. I never removed it, because I needed it when fixing all the lirc_zilog.c ref counting. IF I got all the lirc_zilog ref counting right, and the upper layers of the kernel never call close() in error, then this if() statement is not needed. I welcome anyone wishing to audit the ref-counting in lirc_zilog. It was mentally exhausting to get to what I think is right. Maybe I just tire easily mentally though. :) -Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/staging/media/lirc/lirc_zilog.c b/drivers/staging/media/lirc/lirc_zilog.c index cc872fb..78ce3b0 100644 --- a/drivers/staging/media/lirc/lirc_zilog.c +++ b/drivers/staging/media/lirc/lirc_zilog.c @@ -1332,10 +1332,8 @@ static int close(struct inode *node, struct file *filep) /* find our IR struct */ struct IR *ir = filep->private_data; - if (ir == NULL) { - dev_err(ir->l.dev, "close: no private_data attached to the file!\n"); + if (ir == NULL) return -ENODEV; - } atomic_dec(&ir->open_count);
Fix a possible null pointer dereference, there is otherwise a risk of a possible null pointer dereference. This was found using a static code analysis program called cppcheck Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se> --- drivers/staging/media/lirc/lirc_zilog.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)