[PATCH] drivers/comedi: copy userspace array safely

[PATCH] drivers/comedi: copy userspace array safely

[Philipp Stanner]

comedi_fops.c utilizes memdup_user() to copy a userspace array. This
does not check for an overflow.

Use the new wrapper memdup_array_user() to copy the array more safely.

Suggested-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Philipp Stanner <pstanner@redhat.com>
---
 drivers/comedi/comedi_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/comedi/comedi_fops.c b/drivers/comedi/comedi_fops.c
index 1548dea15df1..1b481731df96 100644
--- a/drivers/comedi/comedi_fops.c
+++ b/drivers/comedi/comedi_fops.c
@@ -1714,8 +1714,8 @@ static int __comedi_get_user_chanlist(struct comedi_device *dev,
 
 	lockdep_assert_held(&dev->mutex);
 	cmd->chanlist = NULL;
-	chanlist = memdup_user(user_chanlist,
-			       cmd->chanlist_len * sizeof(unsigned int));
+	chanlist = memdup_array_user(user_chanlist,
+				     cmd->chanlist_len, sizeof(unsigned int));
 	if (IS_ERR(chanlist))
 		return PTR_ERR(chanlist);
 
-- 
2.41.0

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

[Greg Kroah-Hartman]

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?

> 
> 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".

thanks,

greg k-h

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

[Philipp Stanner]

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.

> 
> > 
> > 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

P.

> 
> thanks,
> 
> greg k-h
> 

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

[Ian Abbott]

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 )=-