| Message ID | 20200619054123.19052-14-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Fri, Jun 19, 2020 at 02:41:19PM +0900, Paul Elder wrote: > V4L2 allows buffer queueing before streamon while libcamera does not. > The compatibility layer thus saves these buffers in a pending queue > until streamon, and then automatically queues them. However, this > pending queue is not cleared when the buffers are freed, so the > following sequence of actions will cause a use-after-free: > > 1. queue buffers > 2. free buffers > - buffers from 1. stay in pending queue but have been freed > 3. queue buffers > 4. streamon > - buffers from 1. are enqueued, then the buffers from 3. are > enqueued. Use-after-free segfault when libcamera tries to handle > the enqueued buffers from 1. > > Fix this by clearing the pending request queue upon buffers being freed. > Also clear the pending request queue on streamOff, for correctness. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > Changes in v2: > - also clear pending request queue on streamOff > - clarify the issue in changelog > --- > src/v4l2/v4l2_camera.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 99d34b9..301a80e 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -148,6 +148,7 @@ void V4L2Camera::freeBuffers() > Stream *stream = *camera_->streams().begin(); > > bufferAllocator_->free(stream); > + pendingRequests_.clear(); Wouldn't it be safer to first clear pendingRequests_, as the requests reference the buffers ? The Request destructor should no access the buffers, so in practice it should be fine, but inverting the order would seem less fragile to me. > } > > FileDescriptor V4L2Camera::getBufferFd(unsigned int index) > @@ -187,7 +188,8 @@ int V4L2Camera::streamOn() > > int V4L2Camera::streamOff() > { > - /* \todo Restore buffers to reqbufs state? */ > + pendingRequests_.clear(); > + Should this be moved after the isRunning_ check ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > if (!isRunning_) > return 0; >
diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp index 99d34b9..301a80e 100644 --- a/src/v4l2/v4l2_camera.cpp +++ b/src/v4l2/v4l2_camera.cpp @@ -148,6 +148,7 @@ void V4L2Camera::freeBuffers() Stream *stream = *camera_->streams().begin(); bufferAllocator_->free(stream); + pendingRequests_.clear(); } FileDescriptor V4L2Camera::getBufferFd(unsigned int index) @@ -187,7 +188,8 @@ int V4L2Camera::streamOn() int V4L2Camera::streamOff() { - /* \todo Restore buffers to reqbufs state? */ + pendingRequests_.clear(); + if (!isRunning_) return 0;
V4L2 allows buffer queueing before streamon while libcamera does not. The compatibility layer thus saves these buffers in a pending queue until streamon, and then automatically queues them. However, this pending queue is not cleared when the buffers are freed, so the following sequence of actions will cause a use-after-free: 1. queue buffers 2. free buffers - buffers from 1. stay in pending queue but have been freed 3. queue buffers 4. streamon - buffers from 1. are enqueued, then the buffers from 3. are enqueued. Use-after-free segfault when libcamera tries to handle the enqueued buffers from 1. Fix this by clearing the pending request queue upon buffers being freed. Also clear the pending request queue on streamOff, for correctness. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> --- Changes in v2: - also clear pending request queue on streamOff - clarify the issue in changelog --- src/v4l2/v4l2_camera.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)