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 <[email protected]>
Signed-off-by: Philipp Stanner <[email protected]>
---
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
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
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
>
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 <[email protected]>
--
-=( Ian Abbott <[email protected]> || 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. || http://www.mev.co.uk )=-