Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755551AbaJNQxl (ORCPT ); Tue, 14 Oct 2014 12:53:41 -0400 Received: from p3plex2out01.prod.phx3.secureserver.net ([184.168.131.12]:47201 "EHLO p3plex2out01.prod.phx3.secureserver.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754779AbaJNQxj convert rfc822-to-8bit (ORCPT ); Tue, 14 Oct 2014 12:53:39 -0400 From: Hartley Sweeten To: Ian Abbott , "driverdev-devel@linuxdriverproject.org" CC: Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , Bernd Porr , "stable@vger.kernel.org" Subject: RE: [PATCH] staging: comedi: (regression) channel list must be set for COMEDI_CMD ioctl Thread-Topic: [PATCH] staging: comedi: (regression) channel list must be set for COMEDI_CMD ioctl Thread-Index: AQHP4wnpNYH9+4ZdNEyejQppx5vVCpwmzf7ggAEg54CAB+lWgA== Date: Tue, 14 Oct 2014 16:53:37 +0000 Message-ID: References: <1412780954-11299-1-git-send-email-abbotti@mev.co.uk> <54364F3F.4060201@mev.co.uk> In-Reply-To: <54364F3F.4060201@mev.co.uk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [184.183.19.121] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday, October 09, 2014 2:03 AM, Ian Abbott wrote: > On 09/10/14 00:13, Hartley Sweeten wrote: >> On Wednesday, October 08, 2014 8:09 AM, Ian Abbott wrote: >>> `do_cmd_ioctl()`, the handler for the `COMEDI_CMD` ioctl can incorrectly >>> call the Comedi subdevice's `do_cmd()` handler with a NULL channel list >>> pointer. This is a regression as the `do_cmd()` handler has never been >>> expected to deal with that, leading to a kernel OOPS when it tries to >>> dereference it. >>> >>> A NULL channel list pointer is allowed for the `COMEDI_CMDTEST` ioctl, >>> handled by `do_cmdtest_ioctl()` and the subdevice's `do_cmdtest()` >>> handler, but not for the `COMEDI_CMD` ioctl and its handlers. >>> >>> Both `do_cmd_ioctl()` and `do_cmdtest_ioctl()` call >>> `__comedi_get_user_chanlist()` to copy the channel list from user memory >>> into dynamically allocated kernel memory and check it for consistency. >>> That function currently returns 0 if the `user_chanlist` parameter >>> (pointing to the channel list in user memory) is NULL. That's fine for >>> `do_cmdtest_ioctl()`, but `do_cmd_ioctl()` incorrectly assumes the >>> kernel copy of the channel list has been set-up correctly. >>> >>> Fix it by not allowing the `user_chanlist` parameter to be NULL in >>> `__comedi_get_user_chanlist()`, and only calling it from >>> `do_cmdtest_ioctl()` if the parameter is non-NULL. >>> >>> Thanks to Bernd Porr for reporting the bug via an initial patch sent >>> privately. >>> >>> Fixes: c6cd0eefb27b ("staging: comedi: comedi_fops: introduce __comedi_get_user_chanlist()") >>> Reported-by: Bernd Porr >>> Signed-off-by: Ian Abbott >>> Cc: Bernd Porr >>> Cc: # 3.15.y 3.16.y 3.17.y >>> --- >>> drivers/staging/comedi/comedi_fops.c | 15 +++++++-------- >>> 1 file changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c >>> index 495969f..a9b7fe5 100644 >>> --- a/drivers/staging/comedi/comedi_fops.c >>> +++ b/drivers/staging/comedi/comedi_fops.c >>> @@ -1462,10 +1462,6 @@ static int __comedi_get_user_chanlist(struct comedi_device *dev, >>> unsigned int *chanlist; >>> int ret; >>> >>> - /* user_chanlist could be NULL for do_cmdtest ioctls */ >>> - if (!user_chanlist) >>> - return 0; >>> - >> >> I think you need a check here to avoid a NULL pointer getting >> passed to the memdup_user(). >> >> If (!user_chanlist || cmd->chanlist_len == 0) >> return -EINVAL; >> >>> chanlist = memdup_user(user_chanlist, >>> cmd->chanlist_len * sizeof(unsigned int)); >>> if (IS_ERR(chanlist)) > > It's fine to pass a NULL pointer to memdup_user(). It won't succeed > (returning ERR_PTR(-EFAULT)), but it's fine. Ah. OK. Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/