lkml.org 
[lkml]   [2019]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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:

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

    \
     
     \ /
      Last update: 2019-03-19 14:06    [W:2.681 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site