From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Todd Poynor <toddpoynor@gmail.com>
Cc: Rob Springer <rspringer@google.com>,
John Joseph <jnjoseph@google.com>,
Ben Chan <benchan@chromium.org>,
devel@driverdev.osuosl.org, Zhongze Hu <frankhu@chromium.org>,
linux-kernel@vger.kernel.org, Simon Que <sque@chromium.org>,
Guenter Roeck <groeck@chromium.org>,
Todd Poynor <toddpoynor@google.com>,
Dmitry Torokhov <dtor@chromium.org>
Subject: Re: [PATCH 17/32] staging: gasket: annotate ioctl arg with __user
Date: Thu, 19 Jul 2018 11:37:43 +0200 [thread overview]
Message-ID: <20180719093743.GC14301@kroah.com> (raw)
In-Reply-To: <20180717205712.29495-18-toddpoynor@gmail.com>
On Tue, Jul 17, 2018 at 01:56:57PM -0700, Todd Poynor wrote:
> From: Todd Poynor <toddpoynor@google.com>
>
> For sparse checking.
Close, but you can do better :)
>
> Reported-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Zhongze Hu <frankhu@chromium.org>
> Signed-off-by: Todd Poynor <toddpoynor@google.com>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> ---
> drivers/staging/gasket/apex_driver.c | 11 ++--
> drivers/staging/gasket/gasket_core.c | 6 ++-
> drivers/staging/gasket/gasket_core.h | 4 +-
> drivers/staging/gasket/gasket_ioctl.c | 72 ++++++++++++++-------------
> drivers/staging/gasket/gasket_ioctl.h | 4 +-
> 5 files changed, 52 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/staging/gasket/apex_driver.c b/drivers/staging/gasket/apex_driver.c
> index 612b3ab803196..c91c5aff5ab9c 100644
> --- a/drivers/staging/gasket/apex_driver.c
> +++ b/drivers/staging/gasket/apex_driver.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2018 Google, Inc.
> */
>
> +#include <linux/compiler.h>
> #include <linux/delay.h>
> #include <linux/fs.h>
> #include <linux/init.h>
> @@ -142,9 +143,9 @@ static int apex_get_status(struct gasket_dev *gasket_dev);
>
> static uint apex_ioctl_check_permissions(struct file *file, uint cmd);
>
> -static long apex_ioctl(struct file *file, uint cmd, ulong arg);
> +static long apex_ioctl(struct file *file, uint cmd, void __user *arg);
>
> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg);
> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg);
Make this a __user pointer to the correct struct type you are handling
here. You know what the type is, use it.
> static int apex_enter_reset(struct gasket_dev *gasket_dev, uint type);
>
> @@ -635,7 +636,7 @@ static uint apex_ioctl_check_permissions(struct file *filp, uint cmd)
> /*
> * Apex-specific ioctl handler.
> */
> -static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
> +static long apex_ioctl(struct file *filp, uint cmd, void __user *arg)
> {
> struct gasket_dev *gasket_dev = filp->private_data;
>
> @@ -655,7 +656,7 @@ static long apex_ioctl(struct file *filp, uint cmd, ulong arg)
> * @gasket_dev: Gasket device pointer.
> * @arg: User ioctl arg, in this case to a apex_gate_clock_ioctl struct.
> */
> -static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
> +static long apex_clock_gating(struct gasket_dev *gasket_dev, void __user *arg)
As above, this should be a different type.
> {
> struct apex_gate_clock_ioctl ibuf;
>
> @@ -663,7 +664,7 @@ static long apex_clock_gating(struct gasket_dev *gasket_dev, ulong arg)
> return 0;
>
> if (allow_sw_clock_gating) {
> - if (copy_from_user(&ibuf, (void __user *)arg, sizeof(ibuf)))
> + if (copy_from_user(&ibuf, arg, sizeof(ibuf)))
> return -EFAULT;
>
> gasket_log_error(
> diff --git a/drivers/staging/gasket/gasket_core.c b/drivers/staging/gasket/gasket_core.c
> index 947b4fcc76970..ff34af42bbe7c 100644
> --- a/drivers/staging/gasket/gasket_core.c
> +++ b/drivers/staging/gasket/gasket_core.c
> @@ -14,6 +14,7 @@
> #include "gasket_page_table.h"
> #include "gasket_sysfs.h"
>
> +#include <linux/compiler.h>
> #include <linux/delay.h>
> #include <linux/fs.h>
> #include <linux/init.h>
> @@ -1823,14 +1824,15 @@ static long gasket_ioctl(struct file *filp, uint cmd, ulong arg)
> * check_and_invoke_callback.
> */
> if (driver_desc->ioctl_handler_cb)
> - return driver_desc->ioctl_handler_cb(filp, cmd, arg);
> + return driver_desc->ioctl_handler_cb(
> + filp, cmd, (void __user *)arg);
You can use a temp variable and then only have to cast things once then,
instead of twice, if you care. Not a big deal.
>
> gasket_log_error(
> gasket_dev, "Received unknown ioctl 0x%x", cmd);
This is a fun way to cause a DoS on your system, you should fix this up
in later changes.
> return -EINVAL;
> }
>
> - return gasket_handle_ioctl(filp, cmd, arg);
> + return gasket_handle_ioctl(filp, cmd, (void __user *)arg);
> }
>
> int gasket_reset(struct gasket_dev *gasket_dev, uint reset_type)
> diff --git a/drivers/staging/gasket/gasket_core.h b/drivers/staging/gasket/gasket_core.h
> index 50ad0c8853183..68b4d2ac9fd6c 100644
> --- a/drivers/staging/gasket/gasket_core.h
> +++ b/drivers/staging/gasket/gasket_core.h
> @@ -315,7 +315,7 @@ struct gasket_dev {
>
> /* Type of the ioctl permissions check callback. See below. */
> typedef int (*gasket_ioctl_permissions_cb_t)(
> - struct file *filp, uint cmd, ulong arg);
> + struct file *filp, uint cmd, void __user *arg);
>
> /*
> * Device type descriptor.
> @@ -549,7 +549,7 @@ struct gasket_driver_desc {
> * return -EINVAL. Should return an error status (either -EINVAL or
> * the error result of the ioctl being handled).
> */
> - long (*ioctl_handler_cb)(struct file *filp, uint cmd, ulong arg);
> + long (*ioctl_handler_cb)(struct file *filp, uint cmd, void __user *arg);
Why are you not using the typedef above?
>
> /*
> * device_status_cb: Callback to determine device health.
> diff --git a/drivers/staging/gasket/gasket_ioctl.c b/drivers/staging/gasket/gasket_ioctl.c
> index d0fa05b8bf1d3..ab2376c32c00b 100644
> --- a/drivers/staging/gasket/gasket_ioctl.c
> +++ b/drivers/staging/gasket/gasket_ioctl.c
> @@ -7,6 +7,7 @@
> #include "gasket_interrupt.h"
> #include "gasket_logging.h"
> #include "gasket_page_table.h"
> +#include <linux/compiler.h>
> #include <linux/fs.h>
> #include <linux/uaccess.h>
>
> @@ -23,17 +24,18 @@
> #endif
>
> static uint gasket_ioctl_check_permissions(struct file *filp, uint cmd);
> -static int gasket_set_event_fd(struct gasket_dev *dev, ulong arg);
> +static int gasket_set_event_fd(struct gasket_dev *dev, void __user *arg);
> static int gasket_read_page_table_size(
> - struct gasket_dev *gasket_dev, ulong arg);
> + struct gasket_dev *gasket_dev, void __user *arg);
> static int gasket_read_simple_page_table_size(
> - struct gasket_dev *gasket_dev, ulong arg);
> + struct gasket_dev *gasket_dev, void __user *arg);
> static int gasket_partition_page_table(
> - struct gasket_dev *gasket_dev, ulong arg);
> -static int gasket_map_buffers(struct gasket_dev *gasket_dev, ulong arg);
> -static int gasket_unmap_buffers(struct gasket_dev *gasket_dev, ulong arg);
> + struct gasket_dev *gasket_dev, void __user *arg);
> +static int gasket_map_buffers(struct gasket_dev *gasket_dev, void __user *arg);
> +static int gasket_unmap_buffers(struct gasket_dev *gasket_dev,
> + void __user *arg);
> static int gasket_config_coherent_allocator(
> - struct gasket_dev *gasket_dev, ulong arg);
> + struct gasket_dev *gasket_dev, void __user *arg);
For most of these you know the real type, please just use it.
> /*
> * standard ioctl dispatch function.
> @@ -43,9 +45,10 @@ static int gasket_config_coherent_allocator(
> *
> * Standard ioctl dispatcher; forwards operations to individual handlers.
> */
> -long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
> +long gasket_handle_ioctl(struct file *filp, uint cmd, void __user *arg)
> {
> struct gasket_dev *gasket_dev;
> + ulong intarg = (ulong)arg;
Ick, really? That's a horrible name for a variable, especially as it
doesn't describe what it is...
and "unsigned long" please, ulong is not a "real" kernel type.
> int retval;
>
> gasket_dev = (struct gasket_dev *)filp->private_data;
> @@ -74,16 +77,16 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
> */
> switch (cmd) {
> case GASKET_IOCTL_RESET:
> - trace_gasket_ioctl_integer_data(arg);
> - retval = gasket_reset(gasket_dev, arg);
> + trace_gasket_ioctl_integer_data(intarg);
> + retval = gasket_reset(gasket_dev, intarg);
> break;
> case GASKET_IOCTL_SET_EVENTFD:
> retval = gasket_set_event_fd(gasket_dev, arg);
> break;
> case GASKET_IOCTL_CLEAR_EVENTFD:
> - trace_gasket_ioctl_integer_data(arg);
> + trace_gasket_ioctl_integer_data(intarg);
> retval = gasket_interrupt_clear_eventfd(
> - gasket_dev->interrupt_data, (int)arg);
> + gasket_dev->interrupt_data, (int)intarg);
See, look at that cast, why would you ever think to cast a variabled
with "int" in the name of it, to an int? Naming is hard :(
> break;
> case GASKET_IOCTL_PARTITION_PAGE_TABLE:
> trace_gasket_ioctl_integer_data(arg);
> @@ -91,7 +94,7 @@ long gasket_handle_ioctl(struct file *filp, uint cmd, ulong arg)
> break;
> case GASKET_IOCTL_NUMBER_PAGE_TABLES:
> trace_gasket_ioctl_integer_data(gasket_dev->num_page_tables);
> - if (copy_to_user((void __user *)arg,
> + if (copy_to_user(arg,
> &gasket_dev->num_page_tables,
Nit, you can move this line up one now.
I'll stop now, you get the idea. This should be broken up a lot.
thanks,
greg k-h
next prev parent reply other threads:[~2018-07-19 9:37 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-17 20:56 [PATCH 00/32 v3] staging: gasket: sundry fixes and fixups Todd Poynor
2018-07-17 20:56 ` [PATCH 01/32] staging: gasket: remove X86 Kconfig restriction Todd Poynor
2018-07-17 20:56 ` [PATCH 02/32] staging: gasket: fix typo in apex_enter_reset Todd Poynor
2018-07-17 20:56 ` [PATCH 03/32] staging: gasket: fix typo in gasket_core.h comments Todd Poynor
2018-07-17 20:56 ` [PATCH 04/32] staging: gasket: whitespace fix in gasket_page_table_init Todd Poynor
2018-07-17 20:56 ` [PATCH 05/32] staging: gasket: remove driver registration on class creation failure Todd Poynor
2018-07-17 20:56 ` [PATCH 06/32] staging: gasket: hold mutex on gasket driver unregistration Todd Poynor
2018-07-17 20:56 ` [PATCH 07/32] staging: gasket: Return EBUSY on mapping create when already in use Todd Poynor
2018-07-17 20:56 ` [PATCH 08/32] staging: gasket: Remove stale pointers on error allocating attr array Todd Poynor
2018-07-17 20:56 ` [PATCH 09/32] staging: gasket: convert gasket_mmap_has_permissions to bool return Todd Poynor
2018-07-17 20:56 ` [PATCH 10/32] staging: gasket: fix gasket_wait_with_reschedule timeout return code Todd Poynor
2018-07-17 20:56 ` [PATCH 11/32] staging: gasket: gasket_wait_with_reschedule use msleep Todd Poynor
2018-07-17 20:56 ` [PATCH 12/32] staging: gasket: gasket_wait_with_reschedule simplify logic Todd Poynor
2018-07-17 20:56 ` [PATCH 13/32] staging: gasket: gasket_wait_with_reschedule use 32 bits of retry count Todd Poynor
2018-07-17 20:56 ` [PATCH 14/32] staging: gasket: bail out of reset sequence on device callback error Todd Poynor
2018-07-17 20:56 ` [PATCH 15/32] staging: gasket: drop gasket_cdev_get_info, use container_of Todd Poynor
2018-07-17 20:56 ` [PATCH 16/32] staging: gasket: always allow root open for write Todd Poynor
2018-07-19 9:29 ` Greg Kroah-Hartman
2018-07-19 23:47 ` Todd Poynor
2018-07-17 20:56 ` [PATCH 17/32] staging: gasket: annotate ioctl arg with __user Todd Poynor
2018-07-19 9:37 ` Greg Kroah-Hartman [this message]
2018-07-20 2:44 ` Todd Poynor
2018-07-20 5:43 ` Greg Kroah-Hartman
2018-07-30 7:00 ` Dan Carpenter
2018-07-17 20:56 ` [PATCH 18/32] staging: gasket: gasket_enable_dev remove unnecessary variable Todd Poynor
2018-07-17 20:56 ` [PATCH 19/32] staging: gasket: remove code for no physical device Todd Poynor
2018-07-17 20:57 ` [PATCH 20/32] staging: gasket: fix class create bug handling Todd Poynor
2018-07-17 20:57 ` [PATCH 21/32] staging: gasket: remove unnecessary code in coherent allocator Todd Poynor
2018-07-17 20:57 ` [PATCH 22/32] staging: gasket: don't treat no device reset callback as an error Todd Poynor
2018-07-17 20:57 ` [PATCH 23/32] staging: gasket: gasket_mmap return error instead of valid BAR index Todd Poynor
2018-07-17 20:57 ` [PATCH 24/32] staging: gasket: apex_clock_gating simplify logic, reduce indentation Todd Poynor
2018-07-17 20:57 ` [PATCH 25/32] staging: gasket: apex_ioctl_check_permissions use bool return type Todd Poynor
2018-07-17 20:57 ` [PATCH 26/32] staging: gasket: gasket page table functions " Todd Poynor
2018-07-17 20:57 ` [PATCH 27/32] staging: gasket: remove else clause after return in if clause Todd Poynor
2018-07-17 20:57 ` [PATCH 28/32] staging: gasket: fix comment syntax in apex.h Todd Poynor
2018-07-17 20:57 ` [PATCH 29/32] staging: gasket: remove unnecessary parens in page table code Todd Poynor
2018-07-17 20:57 ` [PATCH 30/32] staging: gasket: gasket_mmap use PAGE_MASK Todd Poynor
2018-07-17 20:57 ` [PATCH 31/32] staging: gasket: remove extra parens in gasket_write_mappable_regions Todd Poynor
2018-07-17 20:57 ` [PATCH 32/32] staging: gasket: fix multi-line comment syntax in gasket_core.h Todd Poynor
-- strict thread matches above, loose matches on Subject: below --
2018-07-17 2:08 [PATCH 00/32 v2] staging: gasket: sundry fixes and fixups Todd Poynor
2018-07-17 2:09 ` [PATCH 17/32] staging: gasket: annotate ioctl arg with __user Todd Poynor
2018-07-17 10:26 ` Dan Carpenter
2018-07-17 18:20 ` Todd Poynor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180719093743.GC14301@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=benchan@chromium.org \
--cc=devel@driverdev.osuosl.org \
--cc=dtor@chromium.org \
--cc=frankhu@chromium.org \
--cc=groeck@chromium.org \
--cc=jnjoseph@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rspringer@google.com \
--cc=sque@chromium.org \
--cc=toddpoynor@gmail.com \
--cc=toddpoynor@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.