[libcamera-devel,v3,2/2] android: CameraDevice: Add more camera3_capture_request validation
diff mbox series

Message ID 20210402024452.1308253-2-hiroh@chromium.org
State Changes Requested
Headers show
Series
  • [libcamera-devel,v3,1/2] android: cameraDevice: Factorize the code of validating camera3_capture_request
Related show

Commit Message

Hirokazu Honda April 2, 2021, 2:44 a.m. UTC 
This adds more validation to camera3_capture_request mainly
about buffer_handle values.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart April 2, 2021, 11:53 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Fri, Apr 02, 2021 at 11:44:52AM +0900, Hirokazu Honda wrote:
> This adds more validation to camera3_capture_request mainly
> about buffer_handle values.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 988c1fd5..8b6032fc 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -263,11 +263,36 @@ bool isValidRequest(camera3_capture_request_t *camera3Request)
>  		return false;
>  	}
>  
> -	if (!camera3Request->num_output_buffers) {
> +	if (!camera3Request->num_output_buffers ||
> +	    !camera3Request->output_buffers) {
>  		LOG(HAL, Error) << "No output buffers provided";
>  		return false;
>  	}
>  
> +	for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
> +		const camera3_stream_buffer_t &outputBuffer =
> +			camera3Request->output_buffers[i];
> +		if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {

I wonder why camera3_stream_buffer_t.buffer is a buffer_handle_t *
instead of a buffer_handle_t, given that buffer_handle_t is itself an
alias for native_handle_t *. An API design oversight maybe, or do you
know if there's a reason ?

> +			LOG(HAL, Error) << "Invalid native handle";
> +			return false;
> +		}
> +
> +		const native_handle_t *handle = *outputBuffer.buffer;
> +		constexpr int kNativeHandleMaxFds = 1024;
> +		if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
> +			LOG(HAL, Error) << "Invalid number of fds: "
> +					<< handle->numFds;
> +			return false;
> +		}
> +
> +		constexpr int kNativeHandleMaxInts = 1024;
> +		if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
> +			LOG(HAL, Error) << "Invalid number of data: "

s/data/ints/

> +					<< handle->numInts;

Should we give a bit more context though, to make the error message more
explicit ? Something along the lines of

			LOG(HAL, Error)
				<< "Invalid number of data (" << handle->numInts
				<< ") in buffer " << i;

Same for the fds.

> +			return false;
> +		}
> +	}
> +
>  	return true;
>  }
>  
> @@ -1800,7 +1825,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
> -	if (isValidRequest(camera3Request))
> +	if (!isValidRequest(camera3Request))

This belongs to the previous patch.

With these small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		return -EINVAL;
>  
>  	/* Start the camera if that's the first request we handle. */
Hirokazu Honda April 3, 2021, 1:38 p.m. UTC | #2 
Hi Laurent, thanks for reviewing.

On Sat, Apr 3, 2021 at 8:54 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Fri, Apr 02, 2021 at 11:44:52AM +0900, Hirokazu Honda wrote:
> > This adds more validation to camera3_capture_request mainly
> > about buffer_handle values.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 29 +++++++++++++++++++++++++++--
> >  1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 988c1fd5..8b6032fc 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -263,11 +263,36 @@ bool isValidRequest(camera3_capture_request_t *camera3Request)
> >               return false;
> >       }
> >
> > -     if (!camera3Request->num_output_buffers) {
> > +     if (!camera3Request->num_output_buffers ||
> > +         !camera3Request->output_buffers) {
> >               LOG(HAL, Error) << "No output buffers provided";
> >               return false;
> >       }
> >
> > +     for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
> > +             const camera3_stream_buffer_t &outputBuffer =
> > +                     camera3Request->output_buffers[i];
> > +             if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {
>
> I wonder why camera3_stream_buffer_t.buffer is a buffer_handle_t *
> instead of a buffer_handle_t, given that buffer_handle_t is itself an
> alias for native_handle_t *. An API design oversight maybe, or do you
> know if there's a reason ?
>

Yeah, I am confused by it. I have no idea about the reason..

> > +                     LOG(HAL, Error) << "Invalid native handle";
> > +                     return false;
> > +             }
> > +
> > +             const native_handle_t *handle = *outputBuffer.buffer;
> > +             constexpr int kNativeHandleMaxFds = 1024;
> > +             if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
> > +                     LOG(HAL, Error) << "Invalid number of fds: "
> > +                                     << handle->numFds;
> > +                     return false;
> > +             }
> > +
> > +             constexpr int kNativeHandleMaxInts = 1024;
> > +             if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
> > +                     LOG(HAL, Error) << "Invalid number of data: "
>
> s/data/ints/
>
> > +                                     << handle->numInts;
>
> Should we give a bit more context though, to make the error message more
> explicit ? Something along the lines of
>
>                         LOG(HAL, Error)
>                                 << "Invalid number of data (" << handle->numInts
>                                 << ") in buffer " << i;
>
> Same for the fds.
>
> > +                     return false;
> > +             }
> > +     }
> > +
> >       return true;
> >  }
> >
> > @@ -1800,7 +1825,7 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> >  {
> > -     if (isValidRequest(camera3Request))
> > +     if (!isValidRequest(camera3Request))
>
> This belongs to the previous patch.
>
> With these small issues addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >               return -EINVAL;
> >
> >       /* Start the camera if that's the first request we handle. */
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 988c1fd5..8b6032fc 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -263,11 +263,36 @@  bool isValidRequest(camera3_capture_request_t *camera3Request)
 		return false;
 	}
 
-	if (!camera3Request->num_output_buffers) {
+	if (!camera3Request->num_output_buffers ||
+	    !camera3Request->output_buffers) {
 		LOG(HAL, Error) << "No output buffers provided";
 		return false;
 	}
 
+	for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
+		const camera3_stream_buffer_t &outputBuffer =
+			camera3Request->output_buffers[i];
+		if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {
+			LOG(HAL, Error) << "Invalid native handle";
+			return false;
+		}
+
+		const native_handle_t *handle = *outputBuffer.buffer;
+		constexpr int kNativeHandleMaxFds = 1024;
+		if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
+			LOG(HAL, Error) << "Invalid number of fds: "
+					<< handle->numFds;
+			return false;
+		}
+
+		constexpr int kNativeHandleMaxInts = 1024;
+		if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
+			LOG(HAL, Error) << "Invalid number of data: "
+					<< handle->numInts;
+			return false;
+		}
+	}
+
 	return true;
 }
 
@@ -1800,7 +1825,7 @@  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
 
 int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
 {
-	if (isValidRequest(camera3Request))
+	if (!isValidRequest(camera3Request))
 		return -EINVAL;
 
 	/* Start the camera if that's the first request we handle. */