Messages in this thread |  | | Date | Sat, 9 Jan 2021 15:20:53 +0800 (GMT+08:00) | From | dinghao.liu@zju ... | Subject | Re: Re: [PATCH] media: v4l2: Fix memleak in videobuf_read_one |
| |
> On 05/01/2021 08:59, Dinghao Liu wrote: > > When videobuf_waiton() fails, we should execute clean > > functions to prevent memleak. It's the same when > > __videobuf_copy_to_user() fails. > > > > Fixes: 7a7d9a89d0307 ("V4L/DVB (6251): Replace video-buf to a more generic approach") > > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> > > --- > > drivers/media/v4l2-core/videobuf-core.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c > > index 606a271bdd2d..0709b75d11cd 100644 > > --- a/drivers/media/v4l2-core/videobuf-core.c > > +++ b/drivers/media/v4l2-core/videobuf-core.c > > @@ -924,8 +924,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q, > > > > /* wait until capture is done */ > > retval = videobuf_waiton(q, q->read_buf, nonblocking, 1); > > - if (0 != retval) > > + if (retval != 0) { > > + q->ops->buf_release(q, q->read_buf); > > + kfree(q->read_buf); > > + q->read_buf = NULL; > > goto done; > > + } > > I'm fairly certain that this is wrong: if waiton returns an error, then > that means that the wait is either interrupted or that we are in non-blocking > mode and no buffer has arrived yet. In that case you just go to done since > there is nothing to clean up. >
I found there was a similar error handling in videobuf_read_zerocopy(), where q->read_buf was freed on failure of videobuf_waiton(), thus I reported this as a memleak. Do you think the error handling in videobuf_read_zerocopy() is right?
> > > > CALL(q, sync, q, q->read_buf); > > > > @@ -940,8 +944,12 @@ ssize_t videobuf_read_one(struct videobuf_queue *q, > > > > /* Copy to userspace */ > > retval = __videobuf_copy_to_user(q, q->read_buf, data, count, nonblocking); > > - if (retval < 0) > > + if (retval < 0) { > > + q->ops->buf_release(q, q->read_buf); > > + kfree(q->read_buf); > > + q->read_buf = NULL; > > goto done; > > I'm not sure about this either: if userspace gave a crappy pointer and this > copy_to_user fails, then that doesn't mean you should release the buffer. > The next read() might have a valid pointer or, more likely, the application > exits or crashes and everything is cleaned up when the filehandle is closed. >
You are right. Let's keep this part as it was for security.
Regards, Dinghao
|  |