Re: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config

From: Arnd Bergmann <arnd@arndb.de>
To: Dragan Cvetic <dragan.cvetic@xilinx.com>
Cc: gregkh <gregkh@linuxfoundation.org>,
	Michal Simek <michal.simek@xilinx.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Derek Kiernan <derek.kiernan@xilinx.com>
Subject: Re: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config
Date: Tue, 19 Mar 2019 14:05:43 +0100	[thread overview]
Message-ID: <CAK8P3a0E_eZb1rSb9suv65n8WUnz-cXhk24aGoR8eNKcHVPtFw@mail.gmail.com> (raw)
In-Reply-To: <1552997064-432700-9-git-send-email-dragan.cvetic@xilinx.com>

On Tue, Mar 19, 2019 at 1:06 PM Dragan Cvetic <dragan.cvetic@xilinx.com> wrote:
> - Add capability to get SD-FEC config data using ioctl
> XSDFEC_GET_CONFIG.
>
> - Add capability to set SD-FEC data order using ioctl
> SDFEC_SET_ORDER. The order of data blocks can change
> from input to output or to be maintained.

Commenting here only on the ABI, not the actual behavior of the driver:

> +static int xsdfec_get_config(struct xsdfec_dev *xsdfec, void __user *arg)
> +{
> +       int err;
> +
> +       err = copy_to_user(arg, &xsdfec->config, sizeof(xsdfec->config));
> +       if (err) {
> +               dev_err(xsdfec->dev, "%s failed for SDFEC%d", __func__,
> +                       xsdfec->config.fec_id);
> +               err = -EFAULT;
> +       }

Try to avoid printing error messages for things that can be triggered from
user space.

>  static int xsdfec_set_turbo(struct xsdfec_dev *xsdfec, void __user *arg)
>  {
>         struct xsdfec_turbo turbo;
> @@ -670,6 +683,67 @@ static int xsdfec_add_ldpc(struct xsdfec_dev *xsdfec, void __user *arg)
>         return ret;
>  }
>
> +static int xsdfec_set_order(struct xsdfec_dev *xsdfec, void __user *arg)
> +{
> +       bool order_invalid;
> +       enum xsdfec_order order = *((enum xsdfec_order *)arg);

Generally speaking, you should never cast between __user pointers
and kernel pointers. It looks like this will actually dereference a
__user pointer, which is a security issue.

Another problem is that the command is defined as

#define XSDFEC_SET_ORDER _IOW(XSDFEC_MAGIC, 8, unsigned long *)

which would indicate an argument of type 'unsigned long __user * __user *',
which is incompatible with 'enum xsdfec_order __user *'. Both enum
and pointer types are variable length and should be avoided in
ioctl commands. Best make this a '__u64 __user *' or '__u32 __user *'.

> + */
> +#define XSDFEC_GET_CONFIG _IOR(XSDFEC_MAGIC, 6, struct xsdfec_config *)
> +/**
>   * DOC: XSDFEC_GET_TURBO
>   * @Parameters
>   *
> @@ -322,4 +335,48 @@ xsdfec_calculate_shared_ldpc_table_entry_size(struct xsdfec_ldpc_params *ldpc,
>   * ioctl that returns SD-FEC turbo param values
>   */
>  #define XSDFEC_GET_TURBO _IOR(XSDFEC_MAGIC, 7, struct xsdfec_turbo *)

Also wrong type, the function takes a 'struct xsdfec_turbo __user *', not a
'struct xsdfec_turbo __user * __user *',

        Arnd