Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <51e495a4-98d7-402c-a1e6-5c24c43e3118@mev.co.uk>
Date:   Fri, 3 Nov 2023 10:25:45 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] drivers/comedi: copy userspace array safely
Content-Language: en-GB
To:     Philipp Stanner <pstanner@redhat.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     H Hartley Sweeten <hsweeten@visionengravers.com>,
        Ivan Orlov <ivan.orlov0322@gmail.com>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>
References: <20231102190848.51376-2-pstanner@redhat.com>
 <2023110348-drained-tameness-b943@gregkh>
 <07a220351baa1e6851b90c961aade0ce6d26bcf6.camel@redhat.com>
From:   Ian Abbott <abbotti@mev.co.uk>
Organization: MEV Ltd.
In-Reply-To: <07a220351baa1e6851b90c961aade0ce6d26bcf6.camel@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2023-11-03 09:11, Philipp Stanner wrote:
> On Fri, 2023-11-03 at 06:53 +0100, Greg Kroah-Hartman wrote:
>> On Thu, Nov 02, 2023 at 08:08:49PM +0100, Philipp Stanner wrote:
>>> comedi_fops.c utilizes memdup_user() to copy a userspace array. This
>>> does not check for an overflow.
>>
>> Is there potential for an overflow today?
> 
> None that I'm aware of, no. This is more about establishing the new
> function as the standard for array-copying, thereby improving
> readability and maybe robustness in case of future changes.

I agree there is no potential for overflow.  The chanlist_len in the 
command is bound checked against the len_chanlist in the comedi 
subdevice in __comedi_get_user_cmd(), and the len_chanlist value is set 
by driver code with no user input.  So it should be fine barring some 
rogue comedi driver.

>>> Use the new wrapper memdup_array_user() to copy the array more safely.
>>
>> How about saying something like:
>>          "Use the new function memdup_array_user() in case things change
>>          in the future which would prevent overflows if something were to
>>          change in the size of the structures".
>>
>> Or something to the affect of "all is good today, but make it easy to be
>> correct in the future as well".
> 
> Yes, good idea. I'll send a better wording

Feel free to add my reviewed by line:

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-