[{"id":16210,"web_url":"https://patchwork.libcamera.org/comment/16210/","msgid":"","date":"2021-04-13T03:10:25","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> This adds a static function to CameraBuffer class that checks if\n> a buffer_handle is valid with a platform dependent framework.\n> For example, the function validates a buffer using\n> cros::CameraBufferManager on ChromeOS.\n> \n> Signed-off-by: Hirokazu Honda \n> Reviewed-by: Jacopo Mondi \n> ---\n> src/android/camera_buffer.h | 6 ++++++\n> src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> 3 files changed, 33 insertions(+)\n> \n> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> index 7e8970b4..ade082c3 100644\n> --- a/src/android/camera_buffer.h\n> +++ b/src/android/camera_buffer.h\n> @@ -20,6 +20,8 @@ public:\n> \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> \t~CameraBuffer();\n> \n> +\tstatic bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> +\n> \tbool isValid() const;\n> \n> \tunsigned int numPlanes() const;\n> @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\t\\\n> CameraBuffer::~CameraBuffer()\t\t\t\t\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> }\t\t\t\t\t\t\t\t\t\\\n> +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer)\t\t\\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\treturn Private::isValidBuffer(buffer)\t\t\t\t\\\n> +}\t\t\t\t\t\t\t\t\t\\\n> bool CameraBuffer::isValid() const\t\t\t\t\t\\\n> {\t\t\t\t\t\t\t\t\t\\\n> \tconst Private *const d = LIBCAMERA_D_PTR();\t\t\t\\\n> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> index 1a4fd5d1..f8449dfd 100644\n> --- a/src/android/mm/cros_camera_buffer.cpp\n> +++ b/src/android/mm/cros_camera_buffer.cpp\n> @@ -24,6 +24,8 @@ public:\n> \t\tbuffer_handle_t camera3Buffer, int flags);\n> \t~Private();\n> \n> +\tstatic bool isValidBuffer(buffer_handle_t camera3Buffer);\n> +\n> \tbool isValid() const { return valid_; }\n> \n> \tunsigned int numPlanes() const;\n> @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> \treturn bufferManager_->GetPlaneSize(handle_, 0);\n> }\n> \n> +/* static */\n> +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> +{\n> +\tcros::CameraBufferManager *bufferManager =\n> +\t\tcros::CameraBufferManager::GetInstance();\n> +\n> +\tint ret = bufferManager->Register(camera3Buffer);\n> +\tif (ret) {\n> +\t\treturn false;\n> +\t}\n\nNo need for braces.\n\n> +\n> +\tbufferManager->Deregister(camera3Buffer);\n\nThis seems quite inefficient, as it will lead to registering all\nbuffers, even the ones we don't need to map to the CPU. For buffers that\nwe need to map to the CPU, we will register and unregister them here,\nand then register them later.\n\nCould we do better than this ?\n\n> +\n> +\treturn true;\n> +}\n> +\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index 929e078a..07a07372 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -24,6 +24,8 @@ public:\n> \t\tbuffer_handle_t camera3Buffer, int flags);\n> \t~Private();\n> \n> +\tstatic bool isValidBuffer(buffer_handle_t camera3Buffer);\n> +\n> \tunsigned int numPlanes() const;\n> \n> \tSpan plane(unsigned int plane);\n> @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> \t\t\t\t maxJpegBufferSize);\n> }\n> \n> +/* static */\n> +bool CameraBuffer::Private::isValidBuffer(\n> +\t[[maybe_unused]] buffer_handle_t camera3Buffer)\n> +{\n> +\treturn true;\n> +}\n> +\n> PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EA020BD224\n\tfor ;\n\tTue, 13 Apr 2021 03:11:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CBB7687F6;\n\tTue, 13 Apr 2021 05:11:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82E04605AE\n\tfor ;\n\tTue, 13 Apr 2021 05:11:15 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04B646F2;\n\tTue, 13 Apr 2021 05:11:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PJ4pemkF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618283475;\n\tbh=hYjrDvFflkCStyiDLU4g0c5FbTKDHX2X8gni/hqEMWo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PJ4pemkFJXdXG8ByjnoalBQnG51cvGbGnfI6bgVsadu9OqeAPwpPXJ2a4BNhlPHND\n\tyq2f72nMUTjg/lhMq/knOQYH+I9bN/aOPn3mK1FEswEMCdvXVKZD0liMwZKZRa9UZO\n\tr97w7lHurjZGWWttjIdiK1rbCGG9ROcTyn9ImbPE=","Date":"Tue, 13 Apr 2021 06:10:25 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408031040.1388568-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210408031040.1388568-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16227,"web_url":"https://patchwork.libcamera.org/comment/16227/","msgid":"","date":"2021-04-13T08:51:41","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > This adds a static function to CameraBuffer class that checks if\n> > a buffer_handle is valid with a platform dependent framework.\n> > For example, the function validates a buffer using\n> > cros::CameraBufferManager on ChromeOS.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > src/android/camera_buffer.h | 6 ++++++\n> > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > 3 files changed, 33 insertions(+)\n> >\n> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > index 7e8970b4..ade082c3 100644\n> > --- a/src/android/camera_buffer.h\n> > +++ b/src/android/camera_buffer.h\n> > @@ -20,6 +20,8 @@ public:\n> > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > ~CameraBuffer();\n> >\n> > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > +\n> > bool isValid() const;\n> >\n> > unsigned int numPlanes() const;\n> > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > CameraBuffer::~CameraBuffer() \\\n> > { \\\n> > } \\\n> > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > +{ \\\n> > + return Private::isValidBuffer(buffer) \\\n> > +} \\\n> > bool CameraBuffer::isValid() const \\\n> > { \\\n> > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > index 1a4fd5d1..f8449dfd 100644\n> > --- a/src/android/mm/cros_camera_buffer.cpp\n> > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > @@ -24,6 +24,8 @@ public:\n> > buffer_handle_t camera3Buffer, int flags);\n> > ~Private();\n> >\n> > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > +\n> > bool isValid() const { return valid_; }\n> >\n> > unsigned int numPlanes() const;\n> > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > return bufferManager_->GetPlaneSize(handle_, 0);\n> > }\n> >\n> > +/* static */\n> > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > +{\n> > + cros::CameraBufferManager *bufferManager =\n> > + cros::CameraBufferManager::GetInstance();\n> > +\n> > + int ret = bufferManager->Register(camera3Buffer);\n> > + if (ret) {\n> > + return false;\n> > + }\n>\n> No need for braces.\n>\n> > +\n> > + bufferManager->Deregister(camera3Buffer);\n>\n> This seems quite inefficient, as it will lead to registering all\n> buffers, even the ones we don't need to map to the CPU. For buffers that\n> we need to map to the CPU, we will register and unregister them here,\n> and then register them later.\n>\n> Could we do better than this ?\n>\n\nWe may want to add verifyBuffer() to CameraBufferManagerInterface to\ndo a cheap verification, basically only do\ncamera_buffer_handle_t::FromBufferHandle().\n+Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n\n-Hiro\n\n\n> > +\n> > + return true;\n> > +}\n> > +\n> > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > index 929e078a..07a07372 100644\n> > --- a/src/android/mm/generic_camera_buffer.cpp\n> > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > @@ -24,6 +24,8 @@ public:\n> > buffer_handle_t camera3Buffer, int flags);\n> > ~Private();\n> >\n> > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > +\n> > unsigned int numPlanes() const;\n> >\n> > Span plane(unsigned int plane);\n> > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > maxJpegBufferSize);\n> > }\n> >\n> > +/* static */\n> > +bool CameraBuffer::Private::isValidBuffer(\n> > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > +{\n> > + return true;\n> > +}\n> > +\n> > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6E7ACBD1F6\n\tfor ;\n\tTue, 13 Apr 2021 08:51:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF431687FE;\n\tTue, 13 Apr 2021 10:51:53 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D961687F3\n\tfor ;\n\tTue, 13 Apr 2021 10:51:52 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id n2so24622571ejy.7\n\tfor ;\n\tTue, 13 Apr 2021 01:51:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lVE4sEy5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=OB4S4D64ALYSQtnp2yU4rxkovguXHxMFhHU/z3kidtw=;\n\tb=lVE4sEy5kIwiM1WCpuNI9OENaHY3GdKm0W++uWcWy6/DwUqErXNmTvMmKOh0nuFcvA\n\tC/uDnDfMif/frzwSTaOR8ZlF2HBVPXmoULEz/s/F3ziNZKcb/tKoChfLt9ZwvVsKKUIl\n\tO0HywuCJ/ggV12GDJGRqBwkViJmUjyrBLNu0Q=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=OB4S4D64ALYSQtnp2yU4rxkovguXHxMFhHU/z3kidtw=;\n\tb=SFEB9BBdf+wX8NahaOOcVZ8ijoZs0Q4xwksWTPS8knCzGrBEIdxIUhoMkP1Oyd7Bxs\n\tZQ380rFDGYNZ5Hc6ehUK7I5Ilk/at6lwEnwF3rLf7N29EmQZMDuqz31EjpBqhiNFu8J8\n\tNDDmAH39HKX1VKTOsvo+1KB6lzsQ+l8Iq4wCUeA4B5pnUxJqNh/k2FeClAkbKTGRWWQz\n\tQOszdBsm/bz3lYv3jk6ub1g1a8UGjvvFDD0plKJMQJDVtNSXzfb640Juc9RbNR/P8vTq\n\tcSrnaYlslJs/Rxln4bqivUCsRcrlLDaZRSFwd8clYTluR2UYkJIztcdpJE8FZOiJk+qy\n\tLIKg==","X-Gm-Message-State":"AOAM533datmJJ4vsZ8WMuT7bsfZualgEz4ioqic2pKZmVq/o9jdhIhQD\n\trJlSNTHFPL0pr4B8pRp0DcCiliCmSPOW5dX75ANDSZxeVPPXhw==","X-Google-Smtp-Source":"ABdhPJw8Nv72CcLccr0Tyn97A/yGfJ/q/XCOX/4+CVz0wCg0m2YpPKEtDvTAjQGE74mx0pJqyJryqL/vuUiml0/wOSo=","X-Received":"by 2002:a17:906:fcc4:: with SMTP id\n\tqx4mr21780798ejb.42.1618303912047; \n\tTue, 13 Apr 2021 01:51:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Tue, 13 Apr 2021 17:51:41 +0900","Message-ID":"","To":"Laurent Pinchart , shik@chromium.org, \n\tRicky Liang , Tomasz Figa ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16313,"web_url":"https://patchwork.libcamera.org/comment/16313/","msgid":"","date":"2021-04-16T13:43:33","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n>\n> On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > This adds a static function to CameraBuffer class that checks if\n> > > a buffer_handle is valid with a platform dependent framework.\n> > > For example, the function validates a buffer using\n> > > cros::CameraBufferManager on ChromeOS.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > Reviewed-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_buffer.h | 6 ++++++\n> > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > 3 files changed, 33 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > index 7e8970b4..ade082c3 100644\n> > > --- a/src/android/camera_buffer.h\n> > > +++ b/src/android/camera_buffer.h\n> > > @@ -20,6 +20,8 @@ public:\n> > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > ~CameraBuffer();\n> > >\n> > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > +\n> > > bool isValid() const;\n> > >\n> > > unsigned int numPlanes() const;\n> > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > CameraBuffer::~CameraBuffer() \\\n> > > { \\\n> > > } \\\n> > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > +{ \\\n> > > + return Private::isValidBuffer(buffer) \\\n> > > +} \\\n> > > bool CameraBuffer::isValid() const \\\n> > > { \\\n> > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > index 1a4fd5d1..f8449dfd 100644\n> > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > @@ -24,6 +24,8 @@ public:\n> > > buffer_handle_t camera3Buffer, int flags);\n> > > ~Private();\n> > >\n> > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > +\n> > > bool isValid() const { return valid_; }\n> > >\n> > > unsigned int numPlanes() const;\n> > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > }\n> > >\n> > > +/* static */\n> > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > +{\n> > > + cros::CameraBufferManager *bufferManager =\n> > > + cros::CameraBufferManager::GetInstance();\n> > > +\n> > > + int ret = bufferManager->Register(camera3Buffer);\n> > > + if (ret) {\n> > > + return false;\n> > > + }\n> >\n> > No need for braces.\n> >\n> > > +\n> > > + bufferManager->Deregister(camera3Buffer);\n> >\n> > This seems quite inefficient, as it will lead to registering all\n> > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > we need to map to the CPU, we will register and unregister them here,\n> > and then register them later.\n> >\n> > Could we do better than this ?\n> >\n>\n> We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> do a cheap verification, basically only do\n> camera_buffer_handle_t::FromBufferHandle().\n> +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n>\n> -Hiro\n\nAdded https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n\n-Hiro\n>\n>\n> > > +\n> > > + return true;\n> > > +}\n> > > +\n> > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > index 929e078a..07a07372 100644\n> > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > @@ -24,6 +24,8 @@ public:\n> > > buffer_handle_t camera3Buffer, int flags);\n> > > ~Private();\n> > >\n> > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > +\n> > > unsigned int numPlanes() const;\n> > >\n> > > Span plane(unsigned int plane);\n> > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > maxJpegBufferSize);\n> > > }\n> > >\n> > > +/* static */\n> > > +bool CameraBuffer::Private::isValidBuffer(\n> > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > +{\n> > > + return true;\n> > > +}\n> > > +\n> > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D9F9BBD236\n\tfor ;\n\tFri, 16 Apr 2021 13:43:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 432426880C;\n\tFri, 16 Apr 2021 15:43:47 +0200 (CEST)","from mail-ej1-x631.google.com (mail-ej1-x631.google.com\n\t[IPv6:2a00:1450:4864:20::631])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0BA7768806\n\tfor ;\n\tFri, 16 Apr 2021 15:43:45 +0200 (CEST)","by mail-ej1-x631.google.com with SMTP id g5so35517524ejx.0\n\tfor ;\n\tFri, 16 Apr 2021 06:43:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OGNn49A+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=fWuftuT/9skjvRc0wY80KJoNlrmWbrNnJ93+iuhCCu8=;\n\tb=OGNn49A+fS8vjgT6mzpuGIx98QUo9v43a3auHmJTaQrJwZ/mew2XsFXeLNdTDud8U6\n\tZ9wwomwD9fJ24erXdnYU8pUmLCnXYAwga9ID+UP8FZVcKvRfUcSUDc8hE3nh+pZUHOHw\n\t0PnneH9Cp0TM82l/J5dtSHHcWowE2uEY94UeU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=fWuftuT/9skjvRc0wY80KJoNlrmWbrNnJ93+iuhCCu8=;\n\tb=ZXYokMEFYLL3JO92mdapyKUZaO7wcipdQrjP4i1mujpFapm+m2guIxc1M9ZtcoWi1X\n\tO8Ep2U861J4bPQGi+lPFjaWybGsZ/FnNotwD6LTF6mZtM+6b3EwvWyxeNoYC8OcIB3lH\n\tYNVfZVoyoQdeQ1pWl4CYGVPPQpJAygL3G3oExU8MO5Q68PqJBxVEYHxhAmbmy0FgWNal\n\tVMR1SkxjMeMwX96ILXrlNT94MiWou3cXv69WUtINzJaLAv2yjE4yO4jo5m/zl7oFcpFO\n\t3bKqO73C0XoKJGD+4jtYh1vioWURjjyylqbVJNw271Yc+7y6sucn+PjHjDrIXvzUIsVA\n\tl29w==","X-Gm-Message-State":"AOAM530Spbf/LHDAKn3EtdbhPRL+w3MROXL61TOwno6H+HupDUZm3YL1\n\tZp9r7GqqwRBJXE6veb52T2ewzxBnQF3o9MbTixANXA==","X-Google-Smtp-Source":"ABdhPJxQOZjdo0bdXE2/clbzdsk0L567RNjLHaNC7Fb6nKx4FmEa3h6rvCwklegM4DdTkaTQIoqlAcus3VxydyuPWO0=","X-Received":"by 2002:a17:906:fcc4:: with SMTP id\n\tqx4mr8250691ejb.42.1618580624659; \n\tFri, 16 Apr 2021 06:43:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 16 Apr 2021 22:43:33 +0900","Message-ID":"","To":"Laurent Pinchart ,\n\tShik Chen , \n\tRicky Liang , Tomasz Figa ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16317,"web_url":"https://patchwork.libcamera.org/comment/16317/","msgid":"","date":"2021-04-16T15:35:14","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > This adds a static function to CameraBuffer class that checks if\n> > > > a buffer_handle is valid with a platform dependent framework.\n> > > > For example, the function validates a buffer using\n> > > > cros::CameraBufferManager on ChromeOS.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > Reviewed-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_buffer.h | 6 ++++++\n> > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > 3 files changed, 33 insertions(+)\n> > > >\n> > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > index 7e8970b4..ade082c3 100644\n> > > > --- a/src/android/camera_buffer.h\n> > > > +++ b/src/android/camera_buffer.h\n> > > > @@ -20,6 +20,8 @@ public:\n> > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > ~CameraBuffer();\n> > > >\n> > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > +\n> > > > bool isValid() const;\n> > > >\n> > > > unsigned int numPlanes() const;\n> > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > CameraBuffer::~CameraBuffer() \\\n> > > > { \\\n> > > > } \\\n> > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > +{ \\\n> > > > + return Private::isValidBuffer(buffer) \\\n> > > > +} \\\n> > > > bool CameraBuffer::isValid() const \\\n> > > > { \\\n> > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > index 1a4fd5d1..f8449dfd 100644\n> > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > @@ -24,6 +24,8 @@ public:\n> > > > buffer_handle_t camera3Buffer, int flags);\n> > > > ~Private();\n> > > >\n> > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > +\n> > > > bool isValid() const { return valid_; }\n> > > >\n> > > > unsigned int numPlanes() const;\n> > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > }\n> > > >\n> > > > +/* static */\n> > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > +{\n> > > > + cros::CameraBufferManager *bufferManager =\n> > > > + cros::CameraBufferManager::GetInstance();\n> > > > +\n> > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > + if (ret) {\n> > > > + return false;\n> > > > + }\n> > >\n> > > No need for braces.\n> > >\n> > > > +\n> > > > + bufferManager->Deregister(camera3Buffer);\n> > >\n> > > This seems quite inefficient, as it will lead to registering all\n> > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > we need to map to the CPU, we will register and unregister them here,\n> > > and then register them later.\n> > >\n> > > Could we do better than this ?\n> > >\n> >\n> > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > do a cheap verification, basically only do\n> > camera_buffer_handle_t::FromBufferHandle().\n> > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> \n> Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n\nCould you explain what kind of errors you envision this will catch ?\nWhen would the camera HAL implementation receive an incorrect buffer\nhandle in the first place ?\n\n> > > > +\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > index 929e078a..07a07372 100644\n> > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > @@ -24,6 +24,8 @@ public:\n> > > > buffer_handle_t camera3Buffer, int flags);\n> > > > ~Private();\n> > > >\n> > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > +\n> > > > unsigned int numPlanes() const;\n> > > >\n> > > > Span plane(unsigned int plane);\n> > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > maxJpegBufferSize);\n> > > > }\n> > > >\n> > > > +/* static */\n> > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > +{\n> > > > + return true;\n> > > > +}\n> > > > +\n> > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D1FCBBD235\n\tfor ;\n\tFri, 16 Apr 2021 15:35:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0186B68812;\n\tFri, 16 Apr 2021 17:35:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6290768806\n\tfor ;\n\tFri, 16 Apr 2021 17:35:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D56535A5;\n\tFri, 16 Apr 2021 17:35:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"We4CfN92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618587316;\n\tbh=lM07HwBoRzaTxDBxyijnC/AWnaNRhHuDkG7wDGq+Hu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=We4CfN92a0bf+UaeXKyxf4+um/KR7ToSE6AvPndun4xadxIefPQp4/DifgxAKwcnf\n\tsJli/kEDxWpuu3OHO6vIrfGeopii7CBnrj4sP17aWOo/9QgE6tyDh3VKVupJm3gtAD\n\t3GQu6xbIYkRfOz2etAPI5F/xcHLFyqwvaSvj+dtA=","Date":"Fri, 16 Apr 2021 18:35:14 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16331,"web_url":"https://patchwork.libcamera.org/comment/16331/","msgid":"","date":"2021-04-17T04:15:21","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > For example, the function validates a buffer using\n> > > > > cros::CameraBufferManager on ChromeOS.\n> > > > >\n> > > > > Signed-off-by: Hirokazu Honda \n> > > > > Reviewed-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > 3 files changed, 33 insertions(+)\n> > > > >\n> > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > index 7e8970b4..ade082c3 100644\n> > > > > --- a/src/android/camera_buffer.h\n> > > > > +++ b/src/android/camera_buffer.h\n> > > > > @@ -20,6 +20,8 @@ public:\n> > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > ~CameraBuffer();\n> > > > >\n> > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > +\n> > > > > bool isValid() const;\n> > > > >\n> > > > > unsigned int numPlanes() const;\n> > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > { \\\n> > > > > } \\\n> > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > +{ \\\n> > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > +} \\\n> > > > > bool CameraBuffer::isValid() const \\\n> > > > > { \\\n> > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > @@ -24,6 +24,8 @@ public:\n> > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > ~Private();\n> > > > >\n> > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > +\n> > > > > bool isValid() const { return valid_; }\n> > > > >\n> > > > > unsigned int numPlanes() const;\n> > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > }\n> > > > >\n> > > > > +/* static */\n> > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > +{\n> > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > +\n> > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > + if (ret) {\n> > > > > + return false;\n> > > > > + }\n> > > >\n> > > > No need for braces.\n> > > >\n> > > > > +\n> > > > > + bufferManager->Deregister(camera3Buffer);\n> > > >\n> > > > This seems quite inefficient, as it will lead to registering all\n> > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > we need to map to the CPU, we will register and unregister them here,\n> > > > and then register them later.\n> > > >\n> > > > Could we do better than this ?\n> > > >\n> > >\n> > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > do a cheap verification, basically only do\n> > > camera_buffer_handle_t::FromBufferHandle().\n> > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> >\n> > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n>\n> Could you explain what kind of errors you envision this will catch ?\n> When would the camera HAL implementation receive an incorrect buffer\n> handle in the first place ?\n>\n\nIt checks a magic number of the structure is the correct one.\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n\nI have no idea about any situation in the product honestly; our test\ntests that processCaptureRequest() fails when an invalid buffer is\npassed.\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n\n-Hiro\n> > > > > +\n> > > > > + return true;\n> > > > > +}\n> > > > > +\n> > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > index 929e078a..07a07372 100644\n> > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > @@ -24,6 +24,8 @@ public:\n> > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > ~Private();\n> > > > >\n> > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > +\n> > > > > unsigned int numPlanes() const;\n> > > > >\n> > > > > Span plane(unsigned int plane);\n> > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > maxJpegBufferSize);\n> > > > > }\n> > > > >\n> > > > > +/* static */\n> > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > +{\n> > > > > + return true;\n> > > > > +}\n> > > > > +\n> > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D25ABBD2E8\n\tfor ;\n\tSat, 17 Apr 2021 04:15:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C6CA6880B;\n\tSat, 17 Apr 2021 06:15:34 +0200 (CEST)","from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com\n\t[IPv6:2a00:1450:4864:20::62b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04DE2602C5\n\tfor ;\n\tSat, 17 Apr 2021 06:15:32 +0200 (CEST)","by mail-ej1-x62b.google.com with SMTP id mh2so23413688ejb.8\n\tfor ;\n\tFri, 16 Apr 2021 21:15:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"bLOnD+Hb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zO9zYcTnu2Q4+BRancQVSwwjXN876UTKfiJsYKqy/Cg=;\n\tb=bLOnD+HbTJcdsl3YZENzY0DrhHexIVELGrFxrTVZxhyHELpckJC4LPljFtGZUmjG/h\n\tUGJrtx3wAwCp/uLmYngb+wo9tMGYpXYGiCnS1Vv4ODqSPgRjHYfxiGQ2MhZNbfleQdXs\n\t2WvHxuc/KQkuTNviAJ51upZzZdmib6wZWKkhQ=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zO9zYcTnu2Q4+BRancQVSwwjXN876UTKfiJsYKqy/Cg=;\n\tb=Dh7KVbuKR5CG6EfJNOUwvJmRwOy375zrBlQVEpvkZDC5ZmiJSlLzbsW+MuAQ4kl0mh\n\tmL4cR5aEXNFKGXo59zIMpFc7Fe9iiDu1COcp3T4G8i+f+fxlJusewwxlrm/8AhRNka/e\n\tKlLdyOcsgXTx57xnj12P5IpqOSX/6vFHMuFFTchjD3YsyGTS6reXgr/9gpryOPvajmSP\n\tbpxnnYymtvDCzwLmYONs4y1S/vbSjJLbrch3K3fP++QL3e0PeCvMjHNnwf9fC5XzYB3t\n\tknSmXmPhRH4qOsQkibDU6TD/afrDCZTrG65FW0JeJBqvLRphxqGhrEwSbjmGb/ivnnbh\n\tdYMQ==","X-Gm-Message-State":"AOAM531WqamHHuxAFe7BwWdDfFoyPzSyg4nXAsO6tP0MP3ilI23TKBhY\n\trul4IlXeTlEgeWttvafQYgJN7KEWc4OAGpDe+UZRog==","X-Google-Smtp-Source":"ABdhPJwAcV5PcCTKbJIkh39ZmbbmLFzZKBWrxqwntLoo1jN9l0s4xIvNcYhV9rWH0A3YUZwFqPbOf+bPrBq7syS9+c4=","X-Received":"by 2002:a17:906:46d6:: with SMTP id\n\tk22mr11115966ejs.243.1618632932583; \n\tFri, 16 Apr 2021 21:15:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Sat, 17 Apr 2021 13:15:21 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16333,"web_url":"https://patchwork.libcamera.org/comment/16333/","msgid":"","date":"2021-04-17T22:50:30","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > For example, the function validates a buffer using\n> > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > >\n> > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > 3 files changed, 33 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > --- a/src/android/camera_buffer.h\n> > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > ~CameraBuffer();\n> > > > > >\n> > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > +\n> > > > > > bool isValid() const;\n> > > > > >\n> > > > > > unsigned int numPlanes() const;\n> > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > { \\\n> > > > > > } \\\n> > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > +{ \\\n> > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > +} \\\n> > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > { \\\n> > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > ~Private();\n> > > > > >\n> > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > +\n> > > > > > bool isValid() const { return valid_; }\n> > > > > >\n> > > > > > unsigned int numPlanes() const;\n> > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > }\n> > > > > >\n> > > > > > +/* static */\n> > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > +{\n> > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > +\n> > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > + if (ret) {\n> > > > > > + return false;\n> > > > > > + }\n> > > > >\n> > > > > No need for braces.\n> > > > >\n> > > > > > +\n> > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > >\n> > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > and then register them later.\n> > > > >\n> > > > > Could we do better than this ?\n> > > >\n> > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > do a cheap verification, basically only do\n> > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > >\n> > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> >\n> > Could you explain what kind of errors you envision this will catch ?\n> > When would the camera HAL implementation receive an incorrect buffer\n> > handle in the first place ?\n> \n> It checks a magic number of the structure is the correct one.\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> \n> I have no idea about any situation in the product honestly; our test\n> tests that processCaptureRequest() fails when an invalid buffer is\n> passed.\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n\nIn the normal case the buffer handle should be valid. What I'm wondering\nis if there are valid use cases (even if they're rare) where the HAL\ncould receive an invalid handle, or if that would be the consequence of\na serious bug in the camera service. In the latter case, wouldn't it be\nbetter to add the check in the camera service just before calling\n.process_capture_request() ? That would cover all HALs in one go.\n\n> > > > > > +\n> > > > > > + return true;\n> > > > > > +}\n> > > > > > +\n> > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > index 929e078a..07a07372 100644\n> > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > ~Private();\n> > > > > >\n> > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > +\n> > > > > > unsigned int numPlanes() const;\n> > > > > >\n> > > > > > Span plane(unsigned int plane);\n> > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > maxJpegBufferSize);\n> > > > > > }\n> > > > > >\n> > > > > > +/* static */\n> > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > +{\n> > > > > > + return true;\n> > > > > > +}\n> > > > > > +\n> > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8DF5EC314E\n\tfor ;\n\tSat, 17 Apr 2021 22:50:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BA0476880B;\n\tSun, 18 Apr 2021 00:50:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D2D1687F3\n\tfor ;\n\tSun, 18 Apr 2021 00:50:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 922FC51E;\n\tSun, 18 Apr 2021 00:50:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sjbLVQHg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618699832;\n\tbh=3AnT9ZMxpKfNDzWm0e0iiMk75EYB8Gfmkup48PdwXEM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sjbLVQHgZkZ96zhAPIK+IIWu0zA9haMCzk2WQyRzMbZfnFTTtmci4mCETM1KwGOaO\n\tbWBi+NfSTPw8yeo0Naf/21YJKYgefUw9le7JQj/G6kujcJ0lZyhpNIdzZYx3EpGVbR\n\tpLoTppC69uGFQsCYAMpxr/6ZtjdrIVY3w9r0jdus=","Date":"Sun, 18 Apr 2021 01:50:30 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16336,"web_url":"https://patchwork.libcamera.org/comment/16336/","msgid":"","date":"2021-04-19T03:14:22","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Ricky and Shik, what do you think?\n\nOn Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > > For example, the function validates a buffer using\n> > > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > > >\n> > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > > 3 files changed, 33 insertions(+)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > ~CameraBuffer();\n> > > > > > >\n> > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > > +\n> > > > > > > bool isValid() const;\n> > > > > > >\n> > > > > > > unsigned int numPlanes() const;\n> > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > { \\\n> > > > > > > } \\\n> > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > > +{ \\\n> > > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > > +} \\\n> > > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > > { \\\n> > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > ~Private();\n> > > > > > >\n> > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > +\n> > > > > > > bool isValid() const { return valid_; }\n> > > > > > >\n> > > > > > > unsigned int numPlanes() const;\n> > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > > }\n> > > > > > >\n> > > > > > > +/* static */\n> > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > > +{\n> > > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > > +\n> > > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > > + if (ret) {\n> > > > > > > + return false;\n> > > > > > > + }\n> > > > > >\n> > > > > > No need for braces.\n> > > > > >\n> > > > > > > +\n> > > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > > >\n> > > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > > and then register them later.\n> > > > > >\n> > > > > > Could we do better than this ?\n> > > > >\n> > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > > do a cheap verification, basically only do\n> > > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > > >\n> > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> > >\n> > > Could you explain what kind of errors you envision this will catch ?\n> > > When would the camera HAL implementation receive an incorrect buffer\n> > > handle in the first place ?\n> >\n> > It checks a magic number of the structure is the correct one.\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> >\n> > I have no idea about any situation in the product honestly; our test\n> > tests that processCaptureRequest() fails when an invalid buffer is\n> > passed.\n> > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n>\n> In the normal case the buffer handle should be valid. What I'm wondering\n> is if there are valid use cases (even if they're rare) where the HAL\n> could receive an invalid handle, or if that would be the consequence of\n> a serious bug in the camera service. In the latter case, wouldn't it be\n> better to add the check in the camera service just before calling\n> .process_capture_request() ? That would cover all HALs in one go.\n>\n\nRicky and Shik, what do you think?\n\n> > > > > > > +\n> > > > > > > + return true;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > index 929e078a..07a07372 100644\n> > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > ~Private();\n> > > > > > >\n> > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > +\n> > > > > > > unsigned int numPlanes() const;\n> > > > > > >\n> > > > > > > Span plane(unsigned int plane);\n> > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > maxJpegBufferSize);\n> > > > > > > }\n> > > > > > >\n> > > > > > > +/* static */\n> > > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > > +{\n> > > > > > > + return true;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F15E9BD812\n\tfor ;\n\tMon, 19 Apr 2021 03:14:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CB30687F3;\n\tMon, 19 Apr 2021 05:14:34 +0200 (CEST)","from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com\n\t[IPv6:2a00:1450:4864:20::62a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 81E4660516\n\tfor ;\n\tMon, 19 Apr 2021 05:14:32 +0200 (CEST)","by mail-ej1-x62a.google.com with SMTP id w3so50635546ejc.4\n\tfor ;\n\tSun, 18 Apr 2021 20:14:32 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"nkSKYbk0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=FyQ07emzxcl3EycCL+1ySPrwCzwPq97zkI3A7jLLzp8=;\n\tb=nkSKYbk03+ovrIDE9Lgyf7NJ2FnR6oWi+h0szsC2SdZ4xrBb0jua4tZuQxR+N5L3qR\n\tPD7IPKh+b8geRwN0gO0L9uZQF0q2s5zRokqiN1JKEHZl0dYcDZ33DId33ZCQdx1+F1lw\n\teI97EiFWJck2nJIfDWg9a5WZE5LlUC6hhyKCo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=FyQ07emzxcl3EycCL+1ySPrwCzwPq97zkI3A7jLLzp8=;\n\tb=JPmx5SazE2jfmtqfJMUHOLLJDzHW4ACqKPH24Cn9xK9vNK7IkVlK55V9z+HGtRc4T2\n\tJ5quVyHSwjS7zdyjlki0vKjb0oynZccP+yuANej+knVZbo+1QQ5o0QBPokuP0CYW1an+\n\tWIRnb7SbiRQvScL5Zzc3CMWArT30lyRO37QKvNYdAnh2Sw5sq/TXxZ9YapryrE8yQNGs\n\tZr2rMu0vax5QH57n75uxi5T3OVbp2ccthVd1a7yxuMtG7USn0t3RaeJvC176t6u9yo8i\n\tzk9ZX17YdPGWootH3EPvUutSlUkI+iRlP1BD/VvY7FT3B60rFI32DonEul5EbCDBosZl\n\t6YnA==","X-Gm-Message-State":"AOAM5320CU6iCED4I/JdabGTPuGYgsZM/MZ/nncFhSojzzN955ILGtfb\n\tcfpmWF5xvLyLmHXPgRpsTiPuQxBK3Ot1l8oq5jSHFw==","X-Google-Smtp-Source":"ABdhPJyiXPJLJG2R8Z0kvyQU3Pf1s9OGXRBHxycA7NR3AXL67v4mCSg3O44w/erwbvKCto7Ojr7jtEvd07ACMrpajl0=","X-Received":"by 2002:a17:906:80d1:: with SMTP id\n\ta17mr20045130ejx.55.1618802072026; \n\tSun, 18 Apr 2021 20:14:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 19 Apr 2021 12:14:22 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16386,"web_url":"https://patchwork.libcamera.org/comment/16386/","msgid":"","date":"2021-04-20T11:42:27","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":72,"url":"https://patchwork.libcamera.org/api/people/72/","name":"Han-lin Chen","email":"hanlinchen@google.com"},"content":"By discussion with Daniel offline,\nThe test is required only by ChromeOS due to the fact that we don't\ntrust other camera clients.\nChrome shouldn't pass an invalid buffer, although we cannot guarantee\nARC, Parallel, or other future clients does the same.\nFor security, the minimum requirement is that the HAL won't crash or\nput a camera image into an unknown buffer.\n\nSince process_capture_request has to return -EINVAL if input is\nmalformed, I think that's why we added this test as a subcase of the\nrequirement.\nhttps://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296\n\n\nOn Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote:\n>\n> Ricky and Shik, what do you think?\n>\n> On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > > > For example, the function validates a buffer using\n> > > > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > > ---\n> > > > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > > > 3 files changed, 33 insertions(+)\n> > > > > > > >\n> > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > ~CameraBuffer();\n> > > > > > > >\n> > > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > > > +\n> > > > > > > > bool isValid() const;\n> > > > > > > >\n> > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > > { \\\n> > > > > > > > } \\\n> > > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > > > +{ \\\n> > > > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > > > +} \\\n> > > > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > > > { \\\n> > > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > ~Private();\n> > > > > > > >\n> > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > +\n> > > > > > > > bool isValid() const { return valid_; }\n> > > > > > > >\n> > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/* static */\n> > > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > > > +{\n> > > > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > > > +\n> > > > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > > > + if (ret) {\n> > > > > > > > + return false;\n> > > > > > > > + }\n> > > > > > >\n> > > > > > > No need for braces.\n> > > > > > >\n> > > > > > > > +\n> > > > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > > > >\n> > > > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > > > and then register them later.\n> > > > > > >\n> > > > > > > Could we do better than this ?\n> > > > > >\n> > > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > > > do a cheap verification, basically only do\n> > > > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > > > >\n> > > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> > > >\n> > > > Could you explain what kind of errors you envision this will catch ?\n> > > > When would the camera HAL implementation receive an incorrect buffer\n> > > > handle in the first place ?\n> > >\n> > > It checks a magic number of the structure is the correct one.\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> > >\n> > > I have no idea about any situation in the product honestly; our test\n> > > tests that processCaptureRequest() fails when an invalid buffer is\n> > > passed.\n> > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n> >\n> > In the normal case the buffer handle should be valid. What I'm wondering\n> > is if there are valid use cases (even if they're rare) where the HAL\n> > could receive an invalid handle, or if that would be the consequence of\n> > a serious bug in the camera service. In the latter case, wouldn't it be\n> > better to add the check in the camera service just before calling\n> > .process_capture_request() ? That would cover all HALs in one go.\n> >\n>\n> Ricky and Shik, what do you think?\n>\n> > > > > > > > +\n> > > > > > > > + return true;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > index 929e078a..07a07372 100644\n> > > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > ~Private();\n> > > > > > > >\n> > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > +\n> > > > > > > > unsigned int numPlanes() const;\n> > > > > > > >\n> > > > > > > > Span plane(unsigned int plane);\n> > > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > maxJpegBufferSize);\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/* static */\n> > > > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > > > +{\n> > > > > > > > + return true;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\n\n\n\n--\nCheers.\nHanlin Chen","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0327BBDB16\n\tfor ;\n\tTue, 20 Apr 2021 11:42:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5565968844;\n\tTue, 20 Apr 2021 13:42:40 +0200 (CEST)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E8BF60516\n\tfor ;\n\tTue, 20 Apr 2021 13:42:38 +0200 (CEST)","by mail-wr1-x42f.google.com with SMTP id e7so28300719wrs.11\n\tfor ;\n\tTue, 20 Apr 2021 04:42:38 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"SIbeLMyw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=50a1/4aRpoZEdCGV2G9FgJVyeeShQY0MPc2Gayosa8M=;\n\tb=SIbeLMywpxPpYfvjj4GvGabui9Dzy1f0FOCBdIICki7gtpUG7ZGXWl3aHi8OJsH1/T\n\t1v/7ejHCczRbUpPxTy3OHaCKMxF81nJWESIGp5A5XyfdEI5LlfzOC2pMEc4T1mW5Tvsg\n\t4Bq2TVg37S6JfFDieboEZdzuUEC9tTS9CVJM9+2sc4dpdsTjLNkpxuTs30RcI+BGSLon\n\thjF8R47bix0hdupSA12P7ZsFYF7xJ5XckxXBrOrSwmCcf/AhO4NU3fA8QO6yEswcA1HW\n\tjKFjsTpOmKdwK5n3uHo4Fkh4YdH+vwQU4Q9fiwZjq7zAncuaTlkDFBhIfHnsG61++PZN\n\t4vkg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=50a1/4aRpoZEdCGV2G9FgJVyeeShQY0MPc2Gayosa8M=;\n\tb=Yibar9wm0Jr0ltKphrGuV7Ebx7jQeXYMzn0n2rN34RihjC9Gw9yi/jJjvYZSEyVUBo\n\tUTMoGjz/zkg6Kz1he4/ujoJP60pgzwcVuJ1Mg5lG9SCRRNTXtG44Z/q+ZNay1bSrb4nS\n\t+hkYq6n3XQNbapIzOm0LejLa0O25YXRHcq1OCPucrYW/PYrSmX0s7uOfgFyHRL5d5HVs\n\tPiPQ8fpIhU9oZtIgOr8383AzLIZPYCOIKczOZvrgaQa6M0YK8YvWIqBvxEaypvbfJC+I\n\tXPOoPy4xd+zNUmRlL06XjgpDbZAjeGuqKlVr7JFCK6pvWYFYzOBafSvF1SVsLuiBaWO+\n\tAnKg==","X-Gm-Message-State":"AOAM532+0nGPQ/JUK7KRN/qfLglgruEel97/owtEh7VO1sYaQkViX0h1\n\tNrJvrRWUfWFApwAguXgQWpC3fikBjkTzFmpkRlxAtw==","X-Google-Smtp-Source":"ABdhPJzYSRaIZig9Iv2mkrJz2vK3Ox9AU0fo0laCHc3AhD75PWPIpsKN+LI9x70sDuiUVMDi5QJhTCOV8O1UFEQoCUM=","X-Received":"by 2002:adf:fc01:: with SMTP id\n\ti1mr19999135wrr.374.1618918957750; \n\tTue, 20 Apr 2021 04:42:37 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Han-lin Chen ","Date":"Tue, 20 Apr 2021 19:42:27 +0800","Message-ID":"","To":"Hirokazu Honda , Daniel Hung-yu Wu ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16397,"web_url":"https://patchwork.libcamera.org/comment/16397/","msgid":"","date":"2021-04-20T20:42:25","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-lin,\n\nOn Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote:\n> By discussion with Daniel offline,\n> The test is required only by ChromeOS due to the fact that we don't\n> trust other camera clients.\n> Chrome shouldn't pass an invalid buffer, although we cannot guarantee\n> ARC, Parallel, or other future clients does the same.\n> For security, the minimum requirement is that the HAL won't crash or\n> put a camera image into an unknown buffer.\n\nDo the other clients (ARC++, Parallel, ...) access the camera HAL\ndirectly, or do they go through the CrOS camera service ?\n\n> Since process_capture_request has to return -EINVAL if input is\n> malformed, I think that's why we added this test as a subcase of the\n> requirement.\n> https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296\n> \n> On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote:\n> >\n> > Ricky and Shik, what do you think?\n> >\n> > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote:\n> > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > > > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > > > > For example, the function validates a buffer using\n> > > > > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > > > ---\n> > > > > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > > > > 3 files changed, 33 insertions(+)\n> > > > > > > > >\n> > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > ~CameraBuffer();\n> > > > > > > > >\n> > > > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > > > > +\n> > > > > > > > > bool isValid() const;\n> > > > > > > > >\n> > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > > > { \\\n> > > > > > > > > } \\\n> > > > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > > > > +{ \\\n> > > > > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > > > > +} \\\n> > > > > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > > > > { \\\n> > > > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > ~Private();\n> > > > > > > > >\n> > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > +\n> > > > > > > > > bool isValid() const { return valid_; }\n> > > > > > > > >\n> > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +/* static */\n> > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > > > > +{\n> > > > > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > > > > +\n> > > > > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > > > > + if (ret) {\n> > > > > > > > > + return false;\n> > > > > > > > > + }\n> > > > > > > >\n> > > > > > > > No need for braces.\n> > > > > > > >\n> > > > > > > > > +\n> > > > > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > > > > >\n> > > > > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > > > > and then register them later.\n> > > > > > > >\n> > > > > > > > Could we do better than this ?\n> > > > > > >\n> > > > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > > > > do a cheap verification, basically only do\n> > > > > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > > > > >\n> > > > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> > > > >\n> > > > > Could you explain what kind of errors you envision this will catch ?\n> > > > > When would the camera HAL implementation receive an incorrect buffer\n> > > > > handle in the first place ?\n> > > >\n> > > > It checks a magic number of the structure is the correct one.\n> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> > > >\n> > > > I have no idea about any situation in the product honestly; our test\n> > > > tests that processCaptureRequest() fails when an invalid buffer is\n> > > > passed.\n> > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n> > >\n> > > In the normal case the buffer handle should be valid. What I'm wondering\n> > > is if there are valid use cases (even if they're rare) where the HAL\n> > > could receive an invalid handle, or if that would be the consequence of\n> > > a serious bug in the camera service. In the latter case, wouldn't it be\n> > > better to add the check in the camera service just before calling\n> > > .process_capture_request() ? That would cover all HALs in one go.\n> > >\n> >\n> > Ricky and Shik, what do you think?\n> >\n> > > > > > > > > +\n> > > > > > > > > + return true;\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > index 929e078a..07a07372 100644\n> > > > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > ~Private();\n> > > > > > > > >\n> > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > +\n> > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > >\n> > > > > > > > > Span plane(unsigned int plane);\n> > > > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > maxJpegBufferSize);\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +/* static */\n> > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > > > > +{\n> > > > > > > > > + return true;\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4F18EBDB16\n\tfor ;\n\tTue, 20 Apr 2021 20:42:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD5FE68839;\n\tTue, 20 Apr 2021 22:42:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEB2460516\n\tfor ;\n\tTue, 20 Apr 2021 22:42:29 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 576BA411;\n\tTue, 20 Apr 2021 22:42:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kyJoKZHc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618951349;\n\tbh=oG3NwM6QayBvJ/lejMZoB5fFEqYqzHAwj44rKFcRS84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kyJoKZHckdJr3NKRdjuVv1672ZQW0KM4rJcXxYdpCR610Fbdos0Ijh7O/ZJLs7mZ5\n\t26rWaHzWRdKSYW1lVVJl89pnV573hGO/I0MEaMQNMPx/xeGdw2b2IiCwZKqL1VLxEU\n\tpQr3a5RA4en5xEHbpGQT0ZkQC9S9KfbPypLpRQzc=","Date":"Tue, 20 Apr 2021 23:42:25 +0300","From":"Laurent Pinchart ","To":"Han-lin Chen ","Message-ID":"","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ,\n\tDaniel Hung-yu Wu ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16506,"web_url":"https://patchwork.libcamera.org/comment/16506/","msgid":"","date":"2021-04-22T08:27:35","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":72,"url":"https://patchwork.libcamera.org/api/people/72/","name":"Han-lin Chen","email":"hanlinchen@google.com"},"content":"Hi Laurent and Hiro,\n\nOn Wed, Apr 21, 2021 at 4:42 AM Laurent Pinchart\n wrote:\n>\n> Hi Han-lin,\n>\n> On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote:\n> > By discussion with Daniel offline,\n> > The test is required only by ChromeOS due to the fact that we don't\n> > trust other camera clients.\n> > Chrome shouldn't pass an invalid buffer, although we cannot guarantee\n> > ARC, Parallel, or other future clients does the same.\n> > For security, the minimum requirement is that the HAL won't crash or\n> > put a camera image into an unknown buffer.\n>\n> Do the other clients (ARC++, Parallel, ...) access the camera HAL\n> directly, or do they go through the CrOS camera service ?\n\nHmm, they do go through the camera service.\nI added patches to verify the buffer before passing buffers down to\nHAL, and remove the magic number test.\nIn this case we don't need to call back CameraBufferManage::IsValid()\nto check the magic number.\n\n>\n> > Since process_capture_request has to return -EINVAL if input is\n> > malformed, I think that's why we added this test as a subcase of the\n> > requirement.\n> > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296\n> >\n> > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote:\n> > >\n> > > Ricky and Shik, what do you think?\n> > >\n> > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote:\n> > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > > > > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > > > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > > > > > For example, the function validates a buffer using\n> > > > > > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > > > > > >\n> > > > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > > > > ---\n> > > > > > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > > > > > 3 files changed, 33 insertions(+)\n> > > > > > > > > >\n> > > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > ~CameraBuffer();\n> > > > > > > > > >\n> > > > > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > > > > > +\n> > > > > > > > > > bool isValid() const;\n> > > > > > > > > >\n> > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > > > > { \\\n> > > > > > > > > > } \\\n> > > > > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > > > > > +{ \\\n> > > > > > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > > > > > +} \\\n> > > > > > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > > > > > { \\\n> > > > > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > ~Private();\n> > > > > > > > > >\n> > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > > +\n> > > > > > > > > > bool isValid() const { return valid_; }\n> > > > > > > > > >\n> > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > +/* static */\n> > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > > > > > +{\n> > > > > > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > > > > > +\n> > > > > > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > > > > > + if (ret) {\n> > > > > > > > > > + return false;\n> > > > > > > > > > + }\n> > > > > > > > >\n> > > > > > > > > No need for braces.\n> > > > > > > > >\n> > > > > > > > > > +\n> > > > > > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > > > > > >\n> > > > > > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > > > > > and then register them later.\n> > > > > > > > >\n> > > > > > > > > Could we do better than this ?\n> > > > > > > >\n> > > > > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > > > > > do a cheap verification, basically only do\n> > > > > > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > > > > > >\n> > > > > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> > > > > >\n> > > > > > Could you explain what kind of errors you envision this will catch ?\n> > > > > > When would the camera HAL implementation receive an incorrect buffer\n> > > > > > handle in the first place ?\n> > > > >\n> > > > > It checks a magic number of the structure is the correct one.\n> > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> > > > >\n> > > > > I have no idea about any situation in the product honestly; our test\n> > > > > tests that processCaptureRequest() fails when an invalid buffer is\n> > > > > passed.\n> > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n> > > >\n> > > > In the normal case the buffer handle should be valid. What I'm wondering\n> > > > is if there are valid use cases (even if they're rare) where the HAL\n> > > > could receive an invalid handle, or if that would be the consequence of\n> > > > a serious bug in the camera service. In the latter case, wouldn't it be\n> > > > better to add the check in the camera service just before calling\n> > > > .process_capture_request() ? That would cover all HALs in one go.\n> > > >\n> > >\n> > > Ricky and Shik, what do you think?\n> > >\n> > > > > > > > > > +\n> > > > > > > > > > + return true;\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > index 929e078a..07a07372 100644\n> > > > > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > ~Private();\n> > > > > > > > > >\n> > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > > +\n> > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > >\n> > > > > > > > > > Span plane(unsigned int plane);\n> > > > > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > > maxJpegBufferSize);\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > +/* static */\n> > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > > > > > +{\n> > > > > > > > > > + return true;\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 740E0BDB15\n\tfor ;\n\tThu, 22 Apr 2021 08:27:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB2996884C;\n\tThu, 22 Apr 2021 10:27:48 +0200 (CEST)","from mail-wm1-x336.google.com (mail-wm1-x336.google.com\n\t[IPv6:2a00:1450:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B72066883E\n\tfor ;\n\tThu, 22 Apr 2021 10:27:47 +0200 (CEST)","by mail-wm1-x336.google.com with SMTP id\n\tf195-20020a1c1fcc0000b029012eb88126d7so2629693wmf.3\n\tfor ;\n\tThu, 22 Apr 2021 01:27:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"Rg7/PnqP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=nYK3lJ77AEi1SWwxG/919JktpVL2B0Y66p60eVIYs9M=;\n\tb=Rg7/PnqPYL+eYwvKjGjVvjkcVlUSgGsUuHh3MYBf8A5rw+fptfTQQ7xHkqVNYn98N8\n\tscTdDkAN3DZ93+zlq1uDxqcbxaVhn0V2h3qlM7bJkp6LbKek03EgngjnGY6FVKnC6cFu\n\t9sGKNo+i9Rw2euokHenq3uoyGGv2E7lctf6xebcaEpvFrwQgM7wkRUBG3xO2f5wHQme8\n\tN7/GF5hydaGktf0giJC8y8VNQRCEaZjM8f9rkXkUCCSfyVUtGQoUoD/wIIxpjxOyEcIV\n\tgqP0C5FJCQETrlxA2OMJm17q5D+hSk3TfJWaJHSkhcdklnZcnS/U07dORLvrHbCnahcW\n\tLRig==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=nYK3lJ77AEi1SWwxG/919JktpVL2B0Y66p60eVIYs9M=;\n\tb=Ku7sSi2YJaR9G4XTYJYDcewTULWOcZ+wYSqScApvo+QM1FNTw9bSFUBQ86MlJSvre2\n\thsn2neJfwojcYA+8qaEOtGDEAtO/QBm+StEmDVOexm8xAhY25erksnyoE0WulydF2DAv\n\tWBHtZ/M7wj2bVIq/EBqQQauCX9QECH5g6nXMSateKTH6DYjVzEkNJSoNrnZvbS/DjO1L\n\tYE6QjpEeirPdIekd6MGVCG7mZZXwGE+12jClnueC81uTmhXA5KMq38K9EoxDKP4PS1Gq\n\tXYCOwEYDivPxVKEIOHESURRQauiYm69m6SrOQ0C57aA12VD87TFi7QFqN1imsVClRg44\n\te4qg==","X-Gm-Message-State":"AOAM530RwT50E+3tODu1iFDSLE6Ub11WAqrW/lrWVe+Hw69nb89Hgoyz\n\tNBs+EyjxjA8CPDFny7GLtjEegefzBptAOKbQd0q82A==","X-Google-Smtp-Source":"ABdhPJx5Pu/VNRoEylRWS9sCUU8Zom+iS49VuMbqGMsktM+d1G6XanjwAZC3inqs6M4u92Vl48AB2fjGkYGttl5nn6A=","X-Received":"by 2002:a05:600c:25ca:: with SMTP id\n\t10mr14572992wml.0.1619080067048; \n\tThu, 22 Apr 2021 01:27:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20210408031040.1388568-1-hiroh@chromium.org>\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Han-lin Chen ","Date":"Thu, 22 Apr 2021 16:27:35 +0800","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ,\n\tDaniel Hung-yu Wu ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16630,"web_url":"https://patchwork.libcamera.org/comment/16630/","msgid":"","date":"2021-04-27T06:59:55","subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-lin,\n\nOn Thu, Apr 22, 2021 at 04:27:35PM +0800, Han-lin Chen wrote:\n> On Wed, Apr 21, 2021 at 4:42 AM Laurent Pinchart wrote:\n> > On Tue, Apr 20, 2021 at 07:42:27PM +0800, Han-lin Chen wrote:\n> > > By discussion with Daniel offline,\n> > > The test is required only by ChromeOS due to the fact that we don't\n> > > trust other camera clients.\n> > > Chrome shouldn't pass an invalid buffer, although we cannot guarantee\n> > > ARC, Parallel, or other future clients does the same.\n> > > For security, the minimum requirement is that the HAL won't crash or\n> > > put a camera image into an unknown buffer.\n> >\n> > Do the other clients (ARC++, Parallel, ...) access the camera HAL\n> > directly, or do they go through the CrOS camera service ?\n> \n> Hmm, they do go through the camera service.\n> I added patches to verify the buffer before passing buffers down to\n> HAL, and remove the magic number test.\n> In this case we don't need to call back CameraBufferManage::IsValid()\n> to check the magic number.\n\nThank you ! Now all the HAL implementations are protected :-)\n\n> > > Since process_capture_request has to return -EINVAL if input is\n> > > malformed, I think that's why we added this test as a subcase of the\n> > > requirement.\n> > > https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#3296\n> > >\n> > > On Mon, Apr 19, 2021 at 11:14 AM Hirokazu Honda wrote:\n> > > >\n> > > > Ricky and Shik, what do you think?\n> > > >\n> > > > On Sun, Apr 18, 2021 at 7:50 AM Laurent Pinchart wrote:\n> > > > > On Sat, Apr 17, 2021 at 01:15:21PM +0900, Hirokazu Honda wrote:\n> > > > > > On Sat, Apr 17, 2021 at 12:35 AM Laurent Pinchart wrote:\n> > > > > > > On Fri, Apr 16, 2021 at 10:43:33PM +0900, Hirokazu Honda wrote:\n> > > > > > > > On Tue, Apr 13, 2021 at 5:51 PM Hirokazu Honda wrote:\n> > > > > > > > > On Tue, Apr 13, 2021 at 12:11 PM Laurent Pinchart wrote:\n> > > > > > > > > > On Thu, Apr 08, 2021 at 12:10:39PM +0900, Hirokazu Honda wrote:\n> > > > > > > > > > > This adds a static function to CameraBuffer class that checks if\n> > > > > > > > > > > a buffer_handle is valid with a platform dependent framework.\n> > > > > > > > > > > For example, the function validates a buffer using\n> > > > > > > > > > > cros::CameraBufferManager on ChromeOS.\n> > > > > > > > > > >\n> > > > > > > > > > > Signed-off-by: Hirokazu Honda \n> > > > > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > > > > > ---\n> > > > > > > > > > > src/android/camera_buffer.h | 6 ++++++\n> > > > > > > > > > > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++++++\n> > > > > > > > > > > src/android/mm/generic_camera_buffer.cpp | 9 +++++++++\n> > > > > > > > > > > 3 files changed, 33 insertions(+)\n> > > > > > > > > > >\n> > > > > > > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > > > > > > index 7e8970b4..ade082c3 100644\n> > > > > > > > > > > --- a/src/android/camera_buffer.h\n> > > > > > > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > > > > > > @@ -20,6 +20,8 @@ public:\n> > > > > > > > > > > CameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > > ~CameraBuffer();\n> > > > > > > > > > >\n> > > > > > > > > > > + static bool isValidBuffer(buffer_hanlde_t camera3Buffer);\n> > > > > > > > > > > +\n> > > > > > > > > > > bool isValid() const;\n> > > > > > > > > > >\n> > > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > > > @@ -38,6 +40,10 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags) \\\n> > > > > > > > > > > CameraBuffer::~CameraBuffer() \\\n> > > > > > > > > > > { \\\n> > > > > > > > > > > } \\\n> > > > > > > > > > > +bool CameraBuffer::isValidBuffer(buffer_handle_t buffer) \\\n> > > > > > > > > > > +{ \\\n> > > > > > > > > > > + return Private::isValidBuffer(buffer) \\\n> > > > > > > > > > > +} \\\n> > > > > > > > > > > bool CameraBuffer::isValid() const \\\n> > > > > > > > > > > { \\\n> > > > > > > > > > > const Private *const d = LIBCAMERA_D_PTR(); \\\n> > > > > > > > > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > > index 1a4fd5d1..f8449dfd 100644\n> > > > > > > > > > > --- a/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > > +++ b/src/android/mm/cros_camera_buffer.cpp\n> > > > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > > ~Private();\n> > > > > > > > > > >\n> > > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > > > +\n> > > > > > > > > > > bool isValid() const { return valid_; }\n> > > > > > > > > > >\n> > > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > > > @@ -133,4 +135,20 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > > > return bufferManager_->GetPlaneSize(handle_, 0);\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > +/* static */\n> > > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(buffer_handle_t camera3Buffer)\n> > > > > > > > > > > +{\n> > > > > > > > > > > + cros::CameraBufferManager *bufferManager =\n> > > > > > > > > > > + cros::CameraBufferManager::GetInstance();\n> > > > > > > > > > > +\n> > > > > > > > > > > + int ret = bufferManager->Register(camera3Buffer);\n> > > > > > > > > > > + if (ret) {\n> > > > > > > > > > > + return false;\n> > > > > > > > > > > + }\n> > > > > > > > > >\n> > > > > > > > > > No need for braces.\n> > > > > > > > > >\n> > > > > > > > > > > +\n> > > > > > > > > > > + bufferManager->Deregister(camera3Buffer);\n> > > > > > > > > >\n> > > > > > > > > > This seems quite inefficient, as it will lead to registering all\n> > > > > > > > > > buffers, even the ones we don't need to map to the CPU. For buffers that\n> > > > > > > > > > we need to map to the CPU, we will register and unregister them here,\n> > > > > > > > > > and then register them later.\n> > > > > > > > > >\n> > > > > > > > > > Could we do better than this ?\n> > > > > > > > >\n> > > > > > > > > We may want to add verifyBuffer() to CameraBufferManagerInterface to\n> > > > > > > > > do a cheap verification, basically only do\n> > > > > > > > > camera_buffer_handle_t::FromBufferHandle().\n> > > > > > > > > +Shik Chen +Ricky Liang +Tomasz Figa how do you think?\n> > > > > > > >\n> > > > > > > > Added https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2831340\n> > > > > > >\n> > > > > > > Could you explain what kind of errors you envision this will catch ?\n> > > > > > > When would the camera HAL implementation receive an incorrect buffer\n> > > > > > > handle in the first place ?\n> > > > > >\n> > > > > > It checks a magic number of the structure is the correct one.\n> > > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/common/camera_buffer_handle.h;l=82;drc=b45faaeee6b4a79906678746a2d619ccea361358\n> > > > > >\n> > > > > > I have no idea about any situation in the product honestly; our test\n> > > > > > tests that processCaptureRequest() fails when an invalid buffer is\n> > > > > > passed.\n> > > > > > https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/camera/camera3_test/camera3_frame_test.cc;l=1397;drc=34d10525925863b7dddb3db830d45365b2d7e25a\n> > > > >\n> > > > > In the normal case the buffer handle should be valid. What I'm wondering\n> > > > > is if there are valid use cases (even if they're rare) where the HAL\n> > > > > could receive an invalid handle, or if that would be the consequence of\n> > > > > a serious bug in the camera service. In the latter case, wouldn't it be\n> > > > > better to add the check in the camera service just before calling\n> > > > > .process_capture_request() ? That would cover all HALs in one go.\n> > > > >\n> > > >\n> > > > Ricky and Shik, what do you think?\n> > > >\n> > > > > > > > > > > +\n> > > > > > > > > > > + return true;\n> > > > > > > > > > > +}\n> > > > > > > > > > > +\n> > > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION\n> > > > > > > > > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > > index 929e078a..07a07372 100644\n> > > > > > > > > > > --- a/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > > +++ b/src/android/mm/generic_camera_buffer.cpp\n> > > > > > > > > > > @@ -24,6 +24,8 @@ public:\n> > > > > > > > > > > buffer_handle_t camera3Buffer, int flags);\n> > > > > > > > > > > ~Private();\n> > > > > > > > > > >\n> > > > > > > > > > > + static bool isValidBuffer(buffer_handle_t camera3Buffer);\n> > > > > > > > > > > +\n> > > > > > > > > > > unsigned int numPlanes() const;\n> > > > > > > > > > >\n> > > > > > > > > > > Span plane(unsigned int plane);\n> > > > > > > > > > > @@ -85,4 +87,11 @@ size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const\n> > > > > > > > > > > maxJpegBufferSize);\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > +/* static */\n> > > > > > > > > > > +bool CameraBuffer::Private::isValidBuffer(\n> > > > > > > > > > > + [[maybe_unused]] buffer_handle_t camera3Buffer)\n> > > > > > > > > > > +{\n> > > > > > > > > > > + return true;\n> > > > > > > > > > > +}\n> > > > > > > > > > > +\n> > > > > > > > > > > PUBLIC_CAMERA_BUFFER_IMPLEMENTATION","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 98505BDCC3\n\tfor ;\n\tTue, 27 Apr 2021 07:00:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 550C1688B6;\n\tTue, 27 Apr 2021 09:00:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C90466885A\n\tfor ;\n\tTue, 27 Apr 2021 09:00:01 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C204E9;\n\tTue, 27 Apr 2021 09:00:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lE5kKiVz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619506801;\n\tbh=rkchDJ1JcpM3AdUcsusnLRyJFtU6oH1TNkn2KXeoKN0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lE5kKiVz5PDDGdCKLgQUYAgTYgRsxXHUQdMwNZJDlQ9/CfbS8ww9hGAzkf4Bj68Fk\n\tEn3vHf/HyJAjNIBHCLCux5SEMPulhlL9QyN7BK8xFfrO6NftfrVSwql1u+1VsP9rdb\n\t1/oSVJX7CfE8J9/p1LoRkz/ewgLo/VKWX/sKx+m4=","Date":"Tue, 27 Apr 2021 09:59:55 +0300","From":"Laurent Pinchart ","To":"Han-lin Chen ","Message-ID":"","References":"\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] android: CameraBuffer: Add a\n\tstatic function to check a buffer validness","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ,\n\tDaniel Hung-yu Wu ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]