[{"id":27528,"web_url":"https://patchwork.libcamera.org/comment/27528/","msgid":"","date":"2023-07-11T09:06:17","subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Wed, Jul 05, 2023 at 12:22:48AM +0100, Kieran Bingham via libcamera-devel wrote:\n> Handling ioctl's within applications is often wrapped with a helper such\n> as xioctl. Unfortunately, there are many instances of xioctl which\n> incorrectly handle the correct size of the ioctl request.\n>\n> This leads to incorrect sign-extension of ioctl's which have bit-31 set,\n\nI would expect this if the function arguments were signed, but does\nsign-extension happens with unsigned arguments as well ?\n\n> and can cause values to be passed into the libcamera's v4l2 adaptation\n> layer which no longer represent the true IOCTL code.\n>\n> Match the implementation of the Linux kernel and ensure that only 32\n> bits of the ioctl request are used by assigning to an unsigned int.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> This is a (simpler) rework of the previous patch [0]:\n> v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions\n>\n> [0] https://patchwork.libcamera.org/patch/18311/\n>\n> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 55ff62cdb430..dfb672080219 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -778,10 +778,20 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \tVIDIOC_STREAMOFF,\n> };\n>\n> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)\n> {\n> \tMutexLocker locker(proxyMutex_);\n>\n> +\t/*\n> +\t * The Linux Kernel only processes 32 bits of an IOCTL.\n> +\t *\n> +\t * Prevent unexpected sign-extensions that could occur if applications\n> +\t * use an unsigned int for the ioctl request, which would sign-extend\n> +\t * to an incorrect value for unsigned longs on 64 bit architectures by\n> +\t * explicitly casting as an unsigned int here.\n> +\t */\n> +\tunsigned int request = longRequest;\n> +\n> \tif (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n> \t\terrno = EFAULT;\n> \t\treturn -1;\n> --\n> 2.34.1\n>","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 56DAFBEFBE\n\tfor ;\n\tTue, 11 Jul 2023 09:06:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EBC061E37;\n\tTue, 11 Jul 2023 11:06:22 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BE8A61E37\n\tfor ;\n\tTue, 11 Jul 2023 11:06:20 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ACE898CC;\n\tTue, 11 Jul 2023 11:05:31 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689066382;\n\tbh=KkFYqXQSlqkNHEBXcJ0159sTO8hNEbOPFdbc00MdkBI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=38ioHRU1lhpNhTe5lX8wMrjGs++XxLQFQUd5uCnNt5VqxdhXUxJDFzJGuuEzsc3+G\n\tYcj53CazoXMLmtAL7oAXWfQzwHLmBfFzuuoetcW2XgZUixjpjchxbmX6vl3qCOU5q7\n\tduxIpcG3jJiLlCjOGBX0FhDgqgD9mEVzN4b94XzjZtyFZrvy7qOT/I3lPy1SYOQxJv\n\tb0S4/9srFb1c1Nzl7OCSpVd28hlJxLwmKkCaYvEY6nDW5hyd7MOWn2CcuGhSRlkRGy\n\tqrxVDTWD1S41T8vz/DAGyFkEgiSk+mKiIK+ulh2zpuwJOc9puQcWuhnTx+4g9ZsUIE\n\tvWcSIhuaca9pQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689066331;\n\tbh=KkFYqXQSlqkNHEBXcJ0159sTO8hNEbOPFdbc00MdkBI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UvQ/7NWopX+VoJ8OHLWeSMpsca5HrxYzrQb1ifFXyYkgLPUcVzlisl85nhUJOl3SX\n\tVrahLGrj82f2pyXE+a+aaWpybd6HZJa8VZ8c/y+PwV7E7jV4U6NChflcwBK/AQ78wA\n\tux5ZQ89S02GO6zbP2BpS3BvaNgY/vc9p0OZsU/WU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UvQ/7NWo\"; dkim-atps=neutral","Date":"Tue, 11 Jul 2023 11:06:17 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27529,"web_url":"https://patchwork.libcamera.org/comment/27529/","msgid":"","date":"2023-07-11T09:20:14","subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 11/07/2023 10:06, Jacopo Mondi wrote:\n> Hi Kieran\n> \n> On Wed, Jul 05, 2023 at 12:22:48AM +0100, Kieran Bingham via libcamera-devel wrote:\n>> Handling ioctl's within applications is often wrapped with a helper such\n>> as xioctl. Unfortunately, there are many instances of xioctl which\n>> incorrectly handle the correct size of the ioctl request.\n>>\n>> This leads to incorrect sign-extension of ioctl's which have bit-31 set,\n> \n> I would expect this if the function arguments were signed, but does\n> sign-extension happens with unsigned arguments as well ?\n\nThe issue is in the caller - not this function. This patch aims to \nhandle things in the same way the kernel does (which only processes 32 \nbits of the data).\n\nFor instance, the linux kernel documentation has a sample implementation \nof xioctl that would cause the fault that this patch fixes:\n\nhttps://www.kernel.org/doc/html/v4.11/media/uapi/v4l/capture.c.html\n\nstatic int xioctl(int fh, int request, void *arg)\n{\n int r;\n\n do {\n r = ioctl(fh, request, arg);\n } while (-1 == r && EINTR == errno);\n\n return r;\n}\n\nIf an application uses that xioctl, with the 'int request' and calls\n\n if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) {\n }\n\nThe VIDIOC_QUERYCAP (0x80685600) gets sign extended into \n(0xffffffff80685600) and then doesn't match the parsing in the V4L2 \nadaptation layer.\n\n\n> \n>> and can cause values to be passed into the libcamera's v4l2 adaptation\n>> layer which no longer represent the true IOCTL code.\n>>\n>> Match the implementation of the Linux kernel and ensure that only 32\n>> bits of the ioctl request are used by assigning to an unsigned int.\n>>\n\nI'll add:\n\nLink: https://github.com/Motion-Project/motion/discussions/1636\n\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> This is a (simpler) rework of the previous patch [0]:\n>> v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions\n>>\n>> [0] https://patchwork.libcamera.org/patch/18311/\n>>\n>> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n>> 1 file changed, 11 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n>> index 55ff62cdb430..dfb672080219 100644\n>> --- a/src/v4l2/v4l2_camera_proxy.cpp\n>> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n>> @@ -778,10 +778,20 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n>> \tVIDIOC_STREAMOFF,\n>> };\n>>\n>> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n>> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)\n>> {\n>> \tMutexLocker locker(proxyMutex_);\n>>\n>> +\t/*\n>> +\t * The Linux Kernel only processes 32 bits of an IOCTL.\n>> +\t *\n>> +\t * Prevent unexpected sign-extensions that could occur if applications\n>> +\t * use an unsigned int for the ioctl request, which would sign-extend\n>> +\t * to an incorrect value for unsigned longs on 64 bit architectures by\n>> +\t * explicitly casting as an unsigned int here.\n>> +\t */\n>> +\tunsigned int request = longRequest;\n>> +\n>> \tif (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n>> \t\terrno = EFAULT;\n>> \t\treturn -1;\n>> --\n>> 2.34.1\n>>","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 29049BDC71\n\tfor ;\n\tTue, 11 Jul 2023 09:20:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7955F628BD;\n\tTue, 11 Jul 2023 11:20:17 +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 D1510628BC\n\tfor ;\n\tTue, 11 Jul 2023 11:20:15 +0200 (CEST)","from [192.168.0.32]\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D53278CC;\n\tTue, 11 Jul 2023 11:19:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689067217;\n\tbh=NVxsftVOlJT2x/yOPwrT7fQiZLdIZ8PaeB6F3A6TdLI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=sSQMBfydHVe/5BYtlBaaqKp2FcCf3TrGAZVzqXQY66UGjfDGIiZNLick0P7rUSrgs\n\twPE9Qy9csuHMjoi+fik4JfsUvcdlKa+UK74dk6iqojxmdwQVFsnhhkCRkcsXahs2TU\n\t28EDaEOcZPa6z3N076pBe8SuilNAneWKSvCjPmWgStXHpW+9inJsjc95oPOmbnbaSU\n\tGCAx3p1EjD5nH7SEG+rXfeCcvCw4G/bB0jVNnmb1hUa0y+w2n3N3fk/Ihp79ozjWYS\n\tP3wqxk99NKDJ3qPpDefbK3LFh4+b2oN092uTvhMfSGCZGX0jB6veOfkDOHvWCXWcc/\n\tKAVs6tKyidl4g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689067167;\n\tbh=NVxsftVOlJT2x/yOPwrT7fQiZLdIZ8PaeB6F3A6TdLI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=AbzSUog3NC2ikvIuaBRbEQ3l/QNe/YBgeaa/7OKb+zhMi+4OSDdk7/crotbwqGLsw\n\tp1PT/+cDbS45LWtbomylPYL2hAOQV3e03uMGfo58fb6hiHNW48gbH9cmCL43nB+mZS\n\tUsImFtFHnhJZ9QgK955IikJKN/po61+tLI+mmsBI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AbzSUog3\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 11 Jul 2023 10:20:14 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27531,"web_url":"https://patchwork.libcamera.org/comment/27531/","msgid":"<168907121618.540247.5287488612648485407@Monstersaurus>","date":"2023-07-11T10:26:56","subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2023-07-11 10:20:14)\n> Hi Jacopo,\n> \n> On 11/07/2023 10:06, Jacopo Mondi wrote:\n> > Hi Kieran\n> > \n> > On Wed, Jul 05, 2023 at 12:22:48AM +0100, Kieran Bingham via libcamera-devel wrote:\n> >> Handling ioctl's within applications is often wrapped with a helper such\n> >> as xioctl. Unfortunately, there are many instances of xioctl which\n> >> incorrectly handle the correct size of the ioctl request.\n> >>\n> >> This leads to incorrect sign-extension of ioctl's which have bit-31 set,\n> > \n> > I would expect this if the function arguments were signed, but does\n> > sign-extension happens with unsigned arguments as well ?\n> \n> The issue is in the caller - not this function. This patch aims to \n> handle things in the same way the kernel does (which only processes 32 \n> bits of the data).\n> \n> For instance, the linux kernel documentation has a sample implementation \n> of xioctl that would cause the fault that this patch fixes:\n> \n> https://www.kernel.org/doc/html/v4.11/media/uapi/v4l/capture.c.html\n> \n> static int xioctl(int fh, int request, void *arg)\n> {\n> int r;\n> \n> do {\n> r = ioctl(fh, request, arg);\n> } while (-1 == r && EINTR == errno);\n> \n> return r;\n> }\n> \n> If an application uses that xioctl, with the 'int request' and calls\n> \n> if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) {\n> }\n> \n> The VIDIOC_QUERYCAP (0x80685600) gets sign extended into \n> (0xffffffff80685600) and then doesn't match the parsing in the V4L2 \n> adaptation layer.\n> \n> \n> > \n> >> and can cause values to be passed into the libcamera's v4l2 adaptation\n> >> layer which no longer represent the true IOCTL code.\n> >>\n> >> Match the implementation of the Linux kernel and ensure that only 32\n> >> bits of the ioctl request are used by assigning to an unsigned int.\n> >>\n> \n> I'll add:\n> \n> Link: https://github.com/Motion-Project/motion/discussions/1636\n> \n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> This is a (simpler) rework of the previous patch [0]:\n> >> v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions\n> >>\n> >> [0] https://patchwork.libcamera.org/patch/18311/\n> >>\n> >> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n> >> 1 file changed, 11 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> >> index 55ff62cdb430..dfb672080219 100644\n> >> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> >> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> >> @@ -778,10 +778,20 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> >> VIDIOC_STREAMOFF,\n> >> };\n> >>\n> >> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> >> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)\n> >> {\n> >> MutexLocker locker(proxyMutex_);\n> >>\n> >> + /*\n> >> + * The Linux Kernel only processes 32 bits of an IOCTL.\n> >> + *\n> >> + * Prevent unexpected sign-extensions that could occur if applications\n> >> + * use an unsigned int for the ioctl request, which would sign-extend\n\nAha, perhaps your comment was here and this should actually read:\n\n\t\"if applications use a signed int for the ioctl request\"\n\nI'll s/unsigned int/signed int/\n\n--\nKieran\n\n\n> >> + * to an incorrect value for unsigned longs on 64 bit architectures by\n> >> + * explicitly casting as an unsigned int here.\n> >> + */\n> >> + unsigned int request = longRequest;\n> >> +\n> >> if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n> >> errno = EFAULT;\n> >> return -1;\n> >> --\n> >> 2.34.1\n> >>","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 2E97EBDC71\n\tfor ;\n\tTue, 11 Jul 2023 10:27:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B47F61E32;\n\tTue, 11 Jul 2023 12:27:01 +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 616F060570\n\tfor ;\n\tTue, 11 Jul 2023 12:26:59 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 77B2A49E;\n\tTue, 11 Jul 2023 12:26:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689071221;\n\tbh=aHAqLfQ23oIgA8S75uIrhM5jkd0eibZWMmWMxRM+ns4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=t5Q5zKpvhBm8kO4/J6XODHMWabCrV4F/UK4sJHSe3/H316MlZdLWnF/tnylh8UCwr\n\tepDPj6Gbgb/tEIKl42e+WW3dDJikJ3Qkm1kupMq9Wum1KQO9EpvWRfTZYC6dY+h4DL\n\t4qKY0Moo7Py+z3rK1YTaffDvqTk2d2TQ+RTgTwtRsmyKq/5TdMcGwr/EmdDFiDPiIE\n\tlF2mRwRCOZMxXm1pqyCeQjZS6UCijZ0ZOQIzlq5VGXptlTNghsXpM148BGS300lzIq\n\tjz6Ne0tezULlEz1LqRKJfBcEmnWmVjsp5dGn2n/zp/f52P3nX+YJrXIQNk7fjlTInv\n\trK/rFp5etmrhg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689071170;\n\tbh=aHAqLfQ23oIgA8S75uIrhM5jkd0eibZWMmWMxRM+ns4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=m0zkcZgVbFnoNKRd70z/qoMPj4bi+rH8+tsX7wXexRo2GspfImVWQPu2Op0gvX/2F\n\tdPvk3iYBQ+mx/1ev6COiAJnDotOPfynJBKsAO3kln5usAzWRFkRhFpHxM6plXmE0Xl\n\t+ScO8jdDk5r+s9DMOARkI13bNMTCXc0hSzEwOYhg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"m0zkcZgV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>\n\t\n\t","To":"Jacopo Mondi ","Date":"Tue, 11 Jul 2023 11:26:56 +0100","Message-ID":"<168907121618.540247.5287488612648485407@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27533,"web_url":"https://patchwork.libcamera.org/comment/27533/","msgid":"","date":"2023-07-11T10:56:56","subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Tue, Jul 11, 2023 at 11:26:56AM +0100, Kieran Bingham wrote:\n> Quoting Kieran Bingham (2023-07-11 10:20:14)\n> > Hi Jacopo,\n> >\n> > On 11/07/2023 10:06, Jacopo Mondi wrote:\n> > > Hi Kieran\n> > >\n> > > On Wed, Jul 05, 2023 at 12:22:48AM +0100, Kieran Bingham via libcamera-devel wrote:\n> > >> Handling ioctl's within applications is often wrapped with a helper such\n> > >> as xioctl. Unfortunately, there are many instances of xioctl which\n> > >> incorrectly handle the correct size of the ioctl request.\n> > >>\n> > >> This leads to incorrect sign-extension of ioctl's which have bit-31 set,\n> > >\n> > > I would expect this if the function arguments were signed, but does\n> > > sign-extension happens with unsigned arguments as well ?\n> >\n> > The issue is in the caller - not this function. This patch aims to\n> > handle things in the same way the kernel does (which only processes 32\n> > bits of the data).\n> >\n> > For instance, the linux kernel documentation has a sample implementation\n> > of xioctl that would cause the fault that this patch fixes:\n> >\n> > https://www.kernel.org/doc/html/v4.11/media/uapi/v4l/capture.c.html\n> >\n> > static int xioctl(int fh, int request, void *arg)\n\nShouldn't this be a long ?\n\n> > {\n> > int r;\n> >\n> > do {\n> > r = ioctl(fh, request, arg);\n> > } while (-1 == r && EINTR == errno);\n> >\n> > return r;\n> > }\n> >\n> > If an application uses that xioctl, with the 'int request' and calls\n> >\n> > if (xioctl(vid_source, VIDIOC_QUERYCAP, &vid_source->cap) < 0) {\n> > }\n> >\n> > The VIDIOC_QUERYCAP (0x80685600) gets sign extended into\n> > (0xffffffff80685600) and then doesn't match the parsing in the V4L2\n> > adaptation layer.\n> >\n> >\n> > >\n> > >> and can cause values to be passed into the libcamera's v4l2 adaptation\n> > >> layer which no longer represent the true IOCTL code.\n> > >>\n> > >> Match the implementation of the Linux kernel and ensure that only 32\n> > >> bits of the ioctl request are used by assigning to an unsigned int.\n> > >>\n> >\n> > I'll add:\n> >\n> > Link: https://github.com/Motion-Project/motion/discussions/1636\n> >\n> > >> Signed-off-by: Kieran Bingham \n> > >> ---\n> > >> This is a (simpler) rework of the previous patch [0]:\n> > >> v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions\n> > >>\n> > >> [0] https://patchwork.libcamera.org/patch/18311/\n> > >>\n> > >> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n> > >> 1 file changed, 11 insertions(+), 1 deletion(-)\n> > >>\n> > >> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > >> index 55ff62cdb430..dfb672080219 100644\n> > >> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > >> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > >> @@ -778,10 +778,20 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> > >> VIDIOC_STREAMOFF,\n> > >> };\n> > >>\n> > >> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> > >> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)\n> > >> {\n> > >> MutexLocker locker(proxyMutex_);\n> > >>\n> > >> + /*\n> > >> + * The Linux Kernel only processes 32 bits of an IOCTL.\n> > >> + *\n> > >> + * Prevent unexpected sign-extensions that could occur if applications\n> > >> + * use an unsigned int for the ioctl request, which would sign-extend\n>\n> Aha, perhaps your comment was here and this should actually read:\n>\n> \t\"if applications use a signed int for the ioctl request\"\n>\n> I'll s/unsigned int/signed int/\n>\n\nThanks, also this bit was confusing\n\nAnyway, the patch looks good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> --\n> Kieran\n>\n>\n> > >> + * to an incorrect value for unsigned longs on 64 bit architectures by\n> > >> + * explicitly casting as an unsigned int here.\n> > >> + */\n> > >> + unsigned int request = longRequest;\n> > >> +\n> > >> if (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n> > >> errno = EFAULT;\n> > >> return -1;\n> > >> --\n> > >> 2.34.1\n> > >>","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 8DD30BEFBE\n\tfor ;\n\tTue, 11 Jul 2023 10:57:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04EB8628C0;\n\tTue, 11 Jul 2023 12:57:01 +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 EE6B860570\n\tfor ;\n\tTue, 11 Jul 2023 12:56:59 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1723549E;\n\tTue, 11 Jul 2023 12:56:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689073021;\n\tbh=mZ3y872XnoDsd+fPtHfbl5pl+H72GlGcseJSGao3ThY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=FpGJM0zL8nLzopiK6t7yuBQ2B4Qatbn999PbmjA0yaAUzujz4PL+xCoXBfWjINBPU\n\thPl/Et3DeZ7SjfqbZfu1GQLifJ1cfinI6UVCd51AF/4BjX0CO3o7NNuiwxlvkcib+1\n\t97BowrpvOGFhjYmqAiIbS9w4k/GwYZCgtaRiAFNGJ9pRxIxgl0TVg8Z/+quFSwMwas\n\tUDq2y+7zNgl0g2omn0QZAepeS47ngbnjgK7+7liGQe7pPPd0F5R93E59qph2Pgoyz6\n\t7hKoL1okIvH4L0SwLxswVOuKhk0QMAHiclTH+z+jy9kIWRoKCRlIT6SDNHXOasuYJr\n\tsx/24etPODB2A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689072971;\n\tbh=mZ3y872XnoDsd+fPtHfbl5pl+H72GlGcseJSGao3ThY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n1BdTC/9mo1wW507ZZ6W1aClA/TPgu3d5l1NtwKyPkZcF2eHyb5bKo+P8uIs6IuMl\n\tr06AJUjToIvErKb7JpG719AK7nk84lJAt2zPt2AR0MEPL+mGl/m6GngDnRG7z5CuDw\n\thlA+vhZcKsV+6ObUVOV7dwRUy7kKx7fRUhchBvOc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"n1BdTC/9\"; dkim-atps=neutral","Date":"Tue, 11 Jul 2023 12:56:56 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t<168907121618.540247.5287488612648485407@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<168907121618.540247.5287488612648485407@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27542,"web_url":"https://patchwork.libcamera.org/comment/27542/","msgid":"","date":"2023-07-11T15:31:37","subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran\n\nOn 7/5/23 4:52 AM, Kieran Bingham via libcamera-devel wrote:\n> Handling ioctl's within applications is often wrapped with a helper such\n> as xioctl. Unfortunately, there are many instances of xioctl which\n> incorrectly handle the correct size of the ioctl request.\n>\n> This leads to incorrect sign-extension of ioctl's which have bit-31 set,\n> and can cause values to be passed into the libcamera's v4l2 adaptation\n> layer which no longer represent the true IOCTL code.\n>\n> Match the implementation of the Linux kernel and ensure that only 32\n> bits of the ioctl request are used by assigning to an unsigned int.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> This is a (simpler) rework of the previous patch [0]:\n> v4l2: v4l2_camera_proxy: Detect ioctl sign-extensions\n>\n> [0] https://patchwork.libcamera.org/patch/18311/\n>\n> src/v4l2/v4l2_camera_proxy.cpp | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 55ff62cdb430..dfb672080219 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -778,10 +778,20 @@ const std::set V4L2CameraProxy::supportedIoctls_ = {\n> \tVIDIOC_STREAMOFF,\n> };\n> \n> -int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long request, void *arg)\n> +int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void *arg)\n\nIt would be worth to add also why we can't accept int32_t request in the \nfunction parameter itself.\n\nOther than that,\n\nReviewed-by: Umang Jain \n\n> {\n> \tMutexLocker locker(proxyMutex_);\n> \n> +\t/*\n> +\t * The Linux Kernel only processes 32 bits of an IOCTL.\n> +\t *\n> +\t * Prevent unexpected sign-extensions that could occur if applications\n> +\t * use an unsigned int for the ioctl request, which would sign-extend\n> +\t * to an incorrect value for unsigned longs on 64 bit architectures by\n> +\t * explicitly casting as an unsigned int here.\n> +\t */\n> +\tunsigned int request = longRequest;\n> +\n> \tif (!arg && (_IOC_DIR(request) & _IOC_WRITE)) {\n> \t\terrno = EFAULT;\n> \t\treturn -1;","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 B2DFBBEFBE\n\tfor ;\n\tTue, 11 Jul 2023 15:31:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04144628C0;\n\tTue, 11 Jul 2023 17:31:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E672460570\n\tfor ;\n\tTue, 11 Jul 2023 17:31:42 +0200 (CEST)","from [192.168.1.108] (unknown [103.251.226.37])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5C3AC9B9;\n\tTue, 11 Jul 2023 17:30:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1689089505;\n\tbh=Mn4EqX/ZoWZGQ0D1Pj6A9UsFoMctJplrXnv4GeQ2zdA=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=vuOILl0n09wkSJONL+eTAKF/DXuTsKWv0LAzGQf11NGlnuSnmd9PVvorJv6C6Ti/X\n\tPbVEL0muVsl1TmUUhKMgU1bFqz1T7VoR0re8JbhCgEVTUxPF4EHrNZN2E1ngfNqUOJ\n\t5MRolHX/QXb7oQKxuq0O2dCy0hMJzAnTyhY3k7m3VvbmPk9nGFu55jVA/5WslS9v5z\n\tH2Ke6xkNDlVMWh0kuQX0DEw1dF9JFxPEYnZO73p24OtW88kEN+aTEAL850ZH/PNdJw\n\tPTVoo7D4iyhYMVRdtS73b5K6Ni02+T3G0c/KuI6TMqUaOXquPSLLT0pREVjtOIz35X\n\tixORtvv+L/bcA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1689089453;\n\tbh=Mn4EqX/ZoWZGQ0D1Pj6A9UsFoMctJplrXnv4GeQ2zdA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=lluHA2FonBkWRaQJS0HdJd2zOZPR6NR9KScR3mi4v53c9DVz0AS84LpTfDxatazwW\n\t2zYUA/dktaQCElMyj7ztLoA5YcX1I7CmMnTpj+us5G5JUM46PQ3jJR8r2M+pFE+94a\n\t8gBYcBSEd3yNxKF2yh9YpTl0DzWFehPxoLbZoVyc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lluHA2Fo\"; dkim-atps=neutral","Message-ID":"","Date":"Tue, 11 Jul 2023 21:01:37 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.7.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>","In-Reply-To":"<20230704232248.3861085-1-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH] v4l2: v4l2_camera_proxy: Prevent\n\tioctl sign-extensions","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","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]