Message ID | 1552997064-432700-9-git-send-email-dragan.cvetic@xilinx.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | misc: xilinx sd-fec driver | expand |
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
> -----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
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c index 088ad9d..e980b70 100644 --- a/drivers/misc/xilinx_sdfec.c +++ b/drivers/misc/xilinx_sdfec.c @@ -309,6 +309,19 @@ static int xsdfec_dev_release(struct inode *iptr, struct file *fptr) return 0; } +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; + } + return err; +} + 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); + + order_invalid = (order != XSDFEC_MAINTAIN_ORDER) && + (order != XSDFEC_OUT_OF_ORDER); + if (order_invalid) { + dev_err(xsdfec->dev, "%s invalid order value %d for SDFEC%d", + __func__, order, xsdfec->config.fec_id); + return -EINVAL; + } + + /* Verify Device has not started */ + if (xsdfec->state == XSDFEC_STARTED) { + dev_err(xsdfec->dev, + "%s attempting to set Order while started for SDFEC%d", + __func__, xsdfec->config.fec_id); + return -EIO; + } + + xsdfec_regwrite(xsdfec, XSDFEC_ORDER_ADDR, order); + + xsdfec->config.order = order; + + return 0; +} + +static int xsdfec_set_bypass(struct xsdfec_dev *xsdfec, bool __user *arg) +{ + bool bypass = *arg; + + /* Verify Device has not started */ + if (xsdfec->state == XSDFEC_STARTED) { + dev_err(xsdfec->dev, + "%s attempting to set bypass while started for SDFEC%d", + __func__, xsdfec->config.fec_id); + return -EIO; + } + + if (bypass) + xsdfec_regwrite(xsdfec, XSDFEC_BYPASS_ADDR, 1); + else + xsdfec_regwrite(xsdfec, XSDFEC_BYPASS_ADDR, 0); + + xsdfec->config.bypass = bypass; + + return 0; +} + +static int xsdfec_is_active(struct xsdfec_dev *xsdfec, bool __user *is_active) +{ + u32 reg_value; + + reg_value = xsdfec_regread(xsdfec, XSDFEC_ACTIVE_ADDR); + /* using a double ! operator instead of casting */ + *is_active = !!(reg_value & XSDFEC_IS_ACTIVITY_SET); + + return 0; +} + static u32 xsdfec_translate_axis_width_cfg_val(enum xsdfec_axis_width axis_width_cfg) { @@ -771,6 +845,9 @@ static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, } switch (cmd) { + case XSDFEC_GET_CONFIG: + rval = xsdfec_get_config(xsdfec, arg); + break; case XSDFEC_SET_TURBO: rval = xsdfec_set_turbo(xsdfec, arg); break; @@ -780,6 +857,15 @@ static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, case XSDFEC_ADD_LDPC_CODE_PARAMS: rval = xsdfec_add_ldpc(xsdfec, arg); break; + case XSDFEC_SET_ORDER: + rval = xsdfec_set_order(xsdfec, arg); + break; + case XSDFEC_SET_BYPASS: + rval = xsdfec_set_bypass(xsdfec, arg); + break; + case XSDFEC_IS_ACTIVE: + rval = xsdfec_is_active(xsdfec, (bool __user *)arg); + break; default: /* Should not get here */ dev_err(xsdfec->dev, "Undefined SDFEC IOCTL"); diff --git a/include/uapi/misc/xilinx_sdfec.h b/include/uapi/misc/xilinx_sdfec.h index b70dbff..c6584a4 100644 --- a/include/uapi/misc/xilinx_sdfec.h +++ b/include/uapi/misc/xilinx_sdfec.h @@ -310,6 +310,19 @@ xsdfec_calculate_shared_ldpc_table_entry_size(struct xsdfec_ldpc_params *ldpc, #define XSDFEC_ADD_LDPC_CODE_PARAMS \ _IOW(XSDFEC_MAGIC, 5, struct xsdfec_ldpc_params *) /** + * DOC: XSDFEC_GET_CONFIG + * @Parameters + * + * @struct xsdfec_config * + * Pointer to the &struct xsdfec_config that contains the current + * configuration settings of the SD-FEC Block + * + * @Description + * + * ioctl that returns SD-FEC core configuration + */ +#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 *) +/** + * DOC: XSDFEC_SET_ORDER + * @Parameters + * + * @struct unsigned long * + * Pointer to the unsigned long that contains a value from the + * @enum xsdfec_order + * + * @Description + * + * ioctl that sets order, if order of blocks can change from input to output + * + * This can only be used when the driver is in the XSDFEC_STOPPED state + */ +#define XSDFEC_SET_ORDER _IOW(XSDFEC_MAGIC, 8, unsigned long *) +/** + * DOC: XSDFEC_SET_BYPASS + * @Parameters + * + * @struct bool * + * Pointer to bool that sets the bypass value, where false results in + * normal operation and false results in the SD-FEC performing the + * configured operations (same number of cycles) but output data matches + * the input data + * + * @Description + * + * ioctl that sets bypass. + * + * This can only be used when the driver is in the XSDFEC_STOPPED state + */ +#define XSDFEC_SET_BYPASS _IOW(XSDFEC_MAGIC, 9, bool *) +/** + * DOC: XSDFEC_IS_ACTIVE + * @Parameters + * + * @struct bool * + * Pointer to bool that returns true if the SD-FEC is processing data + * + * @Description + * + * ioctl that determines if SD-FEC is processing data + */ +#define XSDFEC_IS_ACTIVE _IOR(XSDFEC_MAGIC, 10, bool *) #endif /* __XILINX_SDFEC_H__ */