linux-kernel - Re: [PATCH] drivers/comedi: copy userspace array safely

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <51e495a4-98d7-402c-a1e6-5c24c43e3118@mev.co.uk>
Date:   Fri, 3 Nov 2023 10:25:45 +0000
From:   Ian Abbott <abbotti@....co.uk>
To:     Philipp Stanner <pstanner@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     H Hartley Sweeten <hsweeten@...ionengravers.com>,
        Ivan Orlov <ivan.orlov0322@...il.com>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        linux-kernel@...r.kernel.org, Dave Airlie <airlied@...hat.com>
Subject: Re: [PATCH] drivers/comedi: copy userspace array safely

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@....co.uk>

-- 
-=( Ian Abbott <abbotti@....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 )=-