* [bug report] [media] lirc: prevent use-after free
@ 2016-11-26 9:57 Dan Carpenter
2016-11-26 11:26 ` Sean Young
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2016-11-26 9:57 UTC (permalink / raw)
To: sean; +Cc: linux-media
Hello Sean Young,
The patch afbb110172b9: "[media] lirc: prevent use-after free" from
Oct 31, 2016, leads to the following static checker warning:
drivers/media/rc/lirc_dev.c:190 lirc_cdev_add()
error: potential null dereference 'cdev'. (cdev_alloc returns null)
drivers/media/rc/lirc_dev.c
158 static int lirc_cdev_add(struct irctl *ir)
159 {
160 int retval = -ENOMEM;
161 struct lirc_driver *d = &ir->d;
162 struct cdev *cdev;
163
164 cdev = cdev_alloc();
165 if (!cdev)
166 goto err_out;
Classic one err bug. Just return directly here. return -ENOMEM is 100%
readable but goto err_out is opaque because you first have to scroll
down to see what err_out does then you have to scroll to the start of
the function to verify that retval is set.
167
168 if (d->fops) {
169 cdev->ops = d->fops;
170 cdev->owner = d->owner;
171 } else {
172 cdev->ops = &lirc_dev_fops;
173 cdev->owner = THIS_MODULE;
174 }
175 retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
176 if (retval)
177 goto err_out;
178
179 retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
180 if (retval) {
181 kobject_put(&cdev->kobj);
This is a double free, isn't it? It should just be goto del_cdev;
182 goto err_out;
183 }
184
185 ir->cdev = cdev;
186
187 return 0;
188
189 err_out:
190 cdev_del(cdev);
Can't pass NULLs to this function.
191 return retval;
192 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] [media] lirc: prevent use-after free
2016-11-26 9:57 [bug report] [media] lirc: prevent use-after free Dan Carpenter
@ 2016-11-26 11:26 ` Sean Young
2016-11-27 19:01 ` [PATCH] [media] lirc: fix error paths in lirc_cdev_add() Sean Young
0 siblings, 1 reply; 3+ messages in thread
From: Sean Young @ 2016-11-26 11:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Hi Dan,
On Sat, Nov 26, 2016 at 12:57:17PM +0300, Dan Carpenter wrote:
> Hello Sean Young,
>
> The patch afbb110172b9: "[media] lirc: prevent use-after free" from
> Oct 31, 2016, leads to the following static checker warning:
>
> drivers/media/rc/lirc_dev.c:190 lirc_cdev_add()
> error: potential null dereference 'cdev'. (cdev_alloc returns null)
>
> drivers/media/rc/lirc_dev.c
> 158 static int lirc_cdev_add(struct irctl *ir)
> 159 {
> 160 int retval = -ENOMEM;
> 161 struct lirc_driver *d = &ir->d;
> 162 struct cdev *cdev;
> 163
> 164 cdev = cdev_alloc();
> 165 if (!cdev)
> 166 goto err_out;
>
> Classic one err bug. Just return directly here. return -ENOMEM is 100%
> readable but goto err_out is opaque because you first have to scroll
> down to see what err_out does then you have to scroll to the start of
> the function to verify that retval is set.
>
> 167
> 168 if (d->fops) {
> 169 cdev->ops = d->fops;
> 170 cdev->owner = d->owner;
> 171 } else {
> 172 cdev->ops = &lirc_dev_fops;
> 173 cdev->owner = THIS_MODULE;
> 174 }
> 175 retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
> 176 if (retval)
> 177 goto err_out;
> 178
> 179 retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
> 180 if (retval) {
> 181 kobject_put(&cdev->kobj);
>
> This is a double free, isn't it? It should just be goto del_cdev;
>
> 182 goto err_out;
> 183 }
> 184
> 185 ir->cdev = cdev;
> 186
> 187 return 0;
> 188
> 189 err_out:
> 190 cdev_del(cdev);
>
> Can't pass NULLs to this function.
>
> 191 return retval;
> 192 }
Oh dear! Thanks for reporting this, you're absolutely right. I'll send
out a patch shortly.
Thanks
Sean
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] [media] lirc: fix error paths in lirc_cdev_add()
2016-11-26 11:26 ` Sean Young
@ 2016-11-27 19:01 ` Sean Young
0 siblings, 0 replies; 3+ messages in thread
From: Sean Young @ 2016-11-27 19:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
"c77d17c0 [media] lirc: use-after free" introduces two problems:
cdev_del() can be called with a NULL argument, and the kobject_put()
path will cause a double free.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Sean Young <sean@mess.org>
---
drivers/media/rc/lirc_dev.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d3039ef..3854809 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -157,13 +157,13 @@ static const struct file_operations lirc_dev_fops = {
static int lirc_cdev_add(struct irctl *ir)
{
- int retval = -ENOMEM;
struct lirc_driver *d = &ir->d;
struct cdev *cdev;
+ int retval;
cdev = cdev_alloc();
if (!cdev)
- goto err_out;
+ return -ENOMEM;
if (d->fops) {
cdev->ops = d->fops;
@@ -177,10 +177,8 @@ static int lirc_cdev_add(struct irctl *ir)
goto err_out;
retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
- if (retval) {
- kobject_put(&cdev->kobj);
+ if (retval)
goto err_out;
- }
ir->cdev = cdev;
--
2.9.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-27 19:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-26 9:57 [bug report] [media] lirc: prevent use-after free Dan Carpenter
2016-11-26 11:26 ` Sean Young
2016-11-27 19:01 ` [PATCH] [media] lirc: fix error paths in lirc_cdev_add() Sean Young
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.