lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config
    Date


    > -----Original Message-----
    > From: Arnd Bergmann [mailto:arnd@arndb.de]
    > Sent: Tuesday 19 March 2019 13:06
    > To: Dragan Cvetic <draganc@xilinx.com>
    > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Linux ARM <linux-arm-kernel@lists.infradead.org>;
    > Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; Derek Kiernan <dkiernan@xilinx.com>
    > Subject: Re: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config
    >
    > 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:

    Will be removed.

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

    Will be removed.

    >
    > > 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 *'.

    Will be updated as proposed.

    >
    > > + */
    > > +#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 *',

    Will be updated as proposed

    >
    > Arnd

    Thank you
    Dragan
    \
     
     \ /
      Last update: 2019-03-19 15:03    [W:4.128 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site