Messages in this thread | | | From | Dragan Cvetic <> | Subject | RE: [PATCH 08/12] misc: xilinx_sdfec: Add ability to get/set config | Date | Tue, 19 Mar 2019 14:02:12 +0000 |
| |
> -----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
| |