linux-kernel - RE: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL0PR02MB568116E14DE884145B641504CB400@BL0PR02MB5681.namprd02.prod.outlook.com>
Date:   Tue, 19 Mar 2019 14:02:12 +0000
From:   Dragan Cvetic <draganc@...inx.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     gregkh <gregkh@...uxfoundation.org>,
        Michal Simek <michals@...inx.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Derek Kiernan <dkiernan@...inx.com>
Subject: RE: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config



> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@...db.de]
> Sent: Tuesday 19 March 2019 13:06
> To: Dragan Cvetic <draganc@...inx.com>
> Cc: gregkh <gregkh@...uxfoundation.org>; Michal Simek <michals@...inx.com>; Linux ARM <linux-arm-kernel@...ts.infradead.org>;
> Linux Kernel Mailing List <linux-kernel@...r.kernel.org>; Derek Kiernan <dkiernan@...inx.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@...inx.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