[libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a static function to check a buffer validness
Han-lin Chen
hanlinchen at google.com
Thu Apr 22 10:27:35 CEST 2021
Hi Laurent and Hiro,
On Wed, Apr 21, 2021 at 4:42 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Han-lin,
>
> On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote:
> > By discussion with Daniel offline,
> > The test is required only by ChromeOS due to the fact that we don't
> > trust other camera clients.
> > Chrome shouldn't pass an invalid buffer, although we cannot guarantee
> > ARC, Parallel, or other future clients does the same.
> > For security, the minimum requirement is that the HAL won't crash or
> > put a camera image into an unknown buffer.
>
> Do the other clients (ARC++, Parallel, ...) access the camera HAL
> directly, or do they go through the CrOS camera service ?
Hmm, they do go through the camera service.
I added patches to verify the buffer before passing buffers down to
HAL, and remove the magic number test.
In this case we don't need to call back CameraBufferManage::IsValid()
to check the magic number.
>
> > Since process_capture_request has to return -EINVAL if input is
> > malformed, I think that's why we added this test as a subcase of the
> > requirement.
> > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296
> >
> > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote:
> > >
> > > Ricky and Shik, what do you think?
> > >
> > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote:
> > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:
> > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:
> > > > > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:
> > > > > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:
> > > > > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:
> > > > > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:
> > > > > > > > > > This adds a static function to CameraBuffer class that checks if
> > > > > > > > > > a buffer_handle is valid with a platform dependent framework.
> > > > > > > > > > For example, the function validates a buffer using
> > > > > > > > > > cros::CameraBufferManager on ChromeOS.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > > > > > ---
> > > > > > > > > > src/android/camera_buffer.h | 6 ++++++
> > > > > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++
> > > > > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++
> > > > > > > > > > 3 files changed, 33 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> > > > > > > > > > index 7e8970b4..ade082c3 100644
> > > > > > > > > > --- a/src/android/camera_buffer.h
> > > > > > > > > > +++ b/src/android/camera_buffer.h
> > > > > > > > > > @@ -20,6 +20,8 @@ public:
> > > > > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);
> > > > > > > > > > ~CameraBuffer();
> > > > > > > > > >
> > > > > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);
> > > > > > > > > > +
> > > > > > > > > > bool isValid() const;
> > > > > > > > > >
> > > > > > > > > > unsigned int numPlanes() const;
> > > > > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \
> > > > > > > > > > CameraBuffer::~CameraBuffer() \
> > > > > > > > > > { \
> > > > > > > > > > } \
> > > > > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \
> > > > > > > > > > +{ \
> > > > > > > > > > + return Private::isValidBuffer(buffer) \
> > > > > > > > > > +} \
> > > > > > > > > > bool CameraBuffer::isValid() const \
> > > > > > > > > > { \
> > > > > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \
> > > > > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > > > > > > > > index 1a4fd5d1..f8449dfd 100644
> > > > > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > > > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > > > > > > > > @@ -24,6 +24,8 @@ public:
> > > > > > > > > > buffer_handle_t camera3Buffer, int flags);
> > > > > > > > > > ~Private();
> > > > > > > > > >
> > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > > > > > > +
> > > > > > > > > > bool isValid() const { return valid_; }
> > > > > > > > > >
> > > > > > > > > > unsigned int numPlanes() const;
> > > > > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +/* static */
> > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)
> > > > > > > > > > +{
> > > > > > > > > > + cros::CameraBufferManager *bufferManager =
> > > > > > > > > > + cros::CameraBufferManager::GetInstance();
> > > > > > > > > > +
> > > > > > > > > > + int ret = bufferManager->Register(camera3Buffer);
> > > > > > > > > > + if (ret) {
> > > > > > > > > > + return false;
> > > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > No need for braces.
> > > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > + bufferManager->Deregister(camera3Buffer);
> > > > > > > > >
> > > > > > > > > This seems quite inefficient, as it will lead to registering all
> > > > > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that
> > > > > > > > > we need to map to the CPU, we will register and unregister them here,
> > > > > > > > > and then register them later.
> > > > > > > > >
> > > > > > > > > Could we do better than this ?
> > > > > > > >
> > > > > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to
> > > > > > > > do a cheap verification, basically only do
> > > > > > > > camera_buffer_handle_t::FromBufferHandle().
> > > > > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?
> > > > > > >
> > > > > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340
> > > > > >
> > > > > > Could you explain what kind of errors you envision this will catch ?
> > > > > > When would the camera HAL implementation receive an incorrect buffer
> > > > > > handle in the first place ?
> > > > >
> > > > > It checks a magic number of the structure is the correct one.
> > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358
> > > > >
> > > > > I have no idea about any situation in the product honestly; our test
> > > > > tests that processCaptureRequest() fails when an invalid buffer is
> > > > > passed.
> > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a
> > > >
> > > > In the normal case the buffer handle should be valid. What I'm wondering
> > > > is if there are valid use cases (even if they're rare) where the HAL
> > > > could receive an invalid handle, or if that would be the consequence of
> > > > a serious bug in the camera service. In the latter case, wouldn't it be
> > > > better to add the check in the camera service just before calling
> > > > .process_capture_request() ? That would cover all HALs in one go.
> > > >
> > >
> > > Ricky and Shik, what do you think?
> > >
> > > > > > > > > > +
> > > > > > > > > > + return true;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > > > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > > > > > > > > index 929e078a..07a07372 100644
> > > > > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > > > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > > > > > > > > @@ -24,6 +24,8 @@ public:
> > > > > > > > > > buffer_handle_t camera3Buffer, int flags);
> > > > > > > > > > ~Private();
> > > > > > > > > >
> > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);
> > > > > > > > > > +
> > > > > > > > > > unsigned int numPlanes() const;
> > > > > > > > > >
> > > > > > > > > > Span<uint8_t> plane(unsigned int plane);
> > > > > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > > > > > > > > maxJpegBufferSize);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > +/* static */
> > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(
> > > > > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)
> > > > > > > > > > +{
> > > > > > > > > > + return true;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
>
> --
> Regards,
>
> Laurent Pinchart
--
Cheers.
Hanlin Chen
More information about the libcamera-devel
mailing list