| Message ID | 20210105075904.27102-1-dinghao.liu@zju.edu.cn |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l2: Fix memleak in videobuf_read_one | expand |
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. > > 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. > + } > > q->read_off += retval; > if (q->read_off == q->read_buf->size) { > Do you have actual proof that this is a memleak? I don't want to mess around with the old videobuf unless you can show me that there is a real bug. Regards, Hans
> 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
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; + } 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; + } q->read_off += retval; if (q->read_off == q->read_buf->size) {
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(-)