Messages in this thread | | | From | Amit Sunil Dhamne <> | Subject | RE: [PATCH 1/3] firmware: xilinx: Add validation check for IOCTL | Date | Wed, 16 Sep 2020 16:53:24 +0000 |
| |
Hi Greg,
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Wednesday, September 16, 2020 4:34 AM > To: Amit Sunil Dhamne <amitsuni@xilinx.com> > Cc: ard.biesheuvel@linaro.org; mingo@kernel.org; > matt@codeblueprint.co.uk; sudeep.holla@arm.com; hkallweit1@gmail.com; > keescook@chromium.org; dmitry.torokhov@gmail.com; Michal Simek > <michals@xilinx.com>; Rajan Vaja <RAJANV@xilinx.com>; Tejas Patel > <TEJASP@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Rajan Vaja > <RAJANV@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH 1/3] firmware: xilinx: Add validation check for IOCTL > > On Wed, Sep 09, 2020 at 05:20:02PM -0700, Amit Sunil Dhamne wrote: > > From: Tejas Patel <tejas.patel@xilinx.com> > > > > Validate IOCTL ID for ZynqMP and Versal before calling > > zynqmp_pm_invoke_fn(). > > > > Signed-off-by: Tejas Patel <tejas.patel@xilinx.com> > > Signed-off-by: Amit Sunil Dhamne <amit.sunil.dhamne@xilinx.com> > > --- > > drivers/firmware/xilinx/zynqmp.c | 117 > > +++++++++++++++++++++++++++++++-------- > > 1 file changed, 95 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/firmware/xilinx/zynqmp.c > > b/drivers/firmware/xilinx/zynqmp.c > > index 8d1ff24..8fe0912 100644 > > --- a/drivers/firmware/xilinx/zynqmp.c > > +++ b/drivers/firmware/xilinx/zynqmp.c > > @@ -514,6 +514,89 @@ int zynqmp_pm_clock_getparent(u32 clock_id, u32 > > *parent_id) EXPORT_SYMBOL_GPL(zynqmp_pm_clock_getparent); > > > > /** > > + * versal_is_valid_ioctl() - Check whether IOCTL ID is valid or not > > +for versal > > + * @ioctl_id: IOCTL ID > > + * > > + * Return: 1 if IOCTL is valid else 0 */ static inline int > > +versal_is_valid_ioctl(u32 ioctl_id) { > > + switch (ioctl_id) { > > + case IOCTL_SD_DLL_RESET: > > + case IOCTL_SET_SD_TAPDELAY: > > + case IOCTL_SET_PLL_FRAC_MODE: > > + case IOCTL_GET_PLL_FRAC_MODE: > > + case IOCTL_SET_PLL_FRAC_DATA: > > + case IOCTL_GET_PLL_FRAC_DATA: > > + case IOCTL_WRITE_GGS: > > + case IOCTL_READ_GGS: > > + case IOCTL_WRITE_PGGS: > > + case IOCTL_READ_PGGS: > > + case IOCTL_SET_BOOT_HEALTH_STATUS: > > + return 1; > > + default: > > + return 0; > > bool is nicer, right? > > > + } > > +} > > + > > +/** > > + * zynqmp_is_valid_ioctl() - Check whether IOCTL ID is valid or not > > + * @ioctl_id: IOCTL ID > > + * > > + * Return: 1 if IOCTL is valid else 0 */ static inline int > > +zynqmp_is_valid_ioctl(u32 ioctl_id) { > > + switch (ioctl_id) { > > + case IOCTL_SD_DLL_RESET: > > + case IOCTL_SET_SD_TAPDELAY: > > + case IOCTL_SET_PLL_FRAC_MODE: > > + case IOCTL_GET_PLL_FRAC_MODE: > > + case IOCTL_SET_PLL_FRAC_DATA: > > + case IOCTL_GET_PLL_FRAC_DATA: > > + case IOCTL_WRITE_GGS: > > + case IOCTL_READ_GGS: > > + case IOCTL_WRITE_PGGS: > > + case IOCTL_READ_PGGS: > > + case IOCTL_SET_BOOT_HEALTH_STATUS: > > + return 1; > > + default: > > + return 0; > > + } > > +} > > + > > +/** > > + * zynqmp_pm_ioctl() - PM IOCTL API for device control and configs > > + * @node_id: Node ID of the device > > + * @ioctl_id: ID of the requested IOCTL > > + * @arg1: Argument 1 to requested IOCTL call > > + * @arg2: Argument 2 to requested IOCTL call > > + * @out: Returned output value > > + * > > + * This function calls IOCTL to firmware for device control and > configuration. > > + * > > + * Return: Returns status, either success or error+reason */ static > > +int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2, > > + u32 *out) > > +{ > > + struct device_node *np; > > + > > + np = of_find_compatible_node(NULL, NULL, "xlnx,versal"); > > + if (np) { > > + if (!versal_is_valid_ioctl(ioctl_id)) > > + return -EINVAL; > > Wrong error value. > > > + } else { > > + if (!zynqmp_is_valid_ioctl(ioctl_id)) > > + return -EINVAL; > > Wrong error value. > > > + } > > + of_node_put(np); > > + > > + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id, arg1, > arg2, > > + out); > > No other checking of ioctl commands and arguments? Brave... > > > +} > > + > > +/** > > * zynqmp_pm_set_pll_frac_mode() - PM API for set PLL mode > > * > > * @clk_id: PLL clock ID > > @@ -525,8 +608,7 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_clock_getparent); > > */ > > int zynqmp_pm_set_pll_frac_mode(u32 clk_id, u32 mode) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, > IOCTL_SET_PLL_FRAC_MODE, > > - clk_id, mode, NULL); > > + return zynqmp_pm_ioctl(0, IOCTL_SET_PLL_FRAC_MODE, clk_id, > > + mode, NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_set_pll_frac_mode); > > > > @@ -542,8 +624,7 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_set_pll_frac_mode); > > */ > > int zynqmp_pm_get_pll_frac_mode(u32 clk_id, u32 *mode) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, > IOCTL_GET_PLL_FRAC_MODE, > > - clk_id, 0, mode); > > + return zynqmp_pm_ioctl(0, IOCTL_GET_PLL_FRAC_MODE, clk_id, 0, > > + mode); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_mode); > > > > @@ -560,8 +641,7 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_mode); > > */ > > int zynqmp_pm_set_pll_frac_data(u32 clk_id, u32 data) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, > IOCTL_SET_PLL_FRAC_DATA, > > - clk_id, data, NULL); > > + return zynqmp_pm_ioctl(0, IOCTL_SET_PLL_FRAC_DATA, clk_id, > > + data, NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_set_pll_frac_data); > > > > @@ -577,8 +657,7 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_set_pll_frac_data); > > */ > > int zynqmp_pm_get_pll_frac_data(u32 clk_id, u32 *data) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, > IOCTL_GET_PLL_FRAC_DATA, > > - clk_id, 0, data); > > + return zynqmp_pm_ioctl(0, IOCTL_GET_PLL_FRAC_DATA, clk_id, 0, > > + data); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_data); > > > > @@ -595,8 +674,8 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_get_pll_frac_data); > > */ > > int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, > IOCTL_SET_SD_TAPDELAY, > > - type, value, NULL); > > + return zynqmp_pm_ioctl(node_id, IOCTL_SET_SD_TAPDELAY, type, > value, > > + NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_tapdelay); > > > > @@ -612,8 +691,7 @@ > EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_tapdelay); > > */ > > int zynqmp_pm_sd_dll_reset(u32 node_id, u32 type) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, > IOCTL_SET_SD_TAPDELAY, > > - type, 0, NULL); > > + return zynqmp_pm_ioctl(node_id, IOCTL_SET_SD_TAPDELAY, type, > > + 0, NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_sd_dll_reset); > > > > @@ -628,8 +706,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_sd_dll_reset); > > */ > > int zynqmp_pm_write_ggs(u32 index, u32 value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, IOCTL_WRITE_GGS, > > - index, value, NULL); > > + return zynqmp_pm_ioctl(0, IOCTL_WRITE_GGS, index, value, > > + NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_write_ggs); > > > > @@ -644,8 +721,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_write_ggs); > > */ > > int zynqmp_pm_read_ggs(u32 index, u32 *value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, IOCTL_READ_GGS, > > - index, 0, value); > > + return zynqmp_pm_ioctl(0, IOCTL_READ_GGS, index, 0, value); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_read_ggs); > > > > @@ -661,8 +737,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_read_ggs); > > */ > > int zynqmp_pm_write_pggs(u32 index, u32 value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, IOCTL_WRITE_PGGS, > index, value, > > - NULL); > > + return zynqmp_pm_ioctl(0, IOCTL_WRITE_PGGS, index, value, > > + NULL); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_write_pggs); > > > > @@ -678,8 +753,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_write_pggs); > > */ > > int zynqmp_pm_read_pggs(u32 index, u32 *value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, IOCTL_READ_PGGS, > index, 0, > > - value); > > + return zynqmp_pm_ioctl(0, IOCTL_READ_PGGS, index, 0, value); > > } > > EXPORT_SYMBOL_GPL(zynqmp_pm_read_pggs); > > > > @@ -694,8 +768,7 @@ EXPORT_SYMBOL_GPL(zynqmp_pm_read_pggs); > > */ > > int zynqmp_pm_set_boot_health_status(u32 value) { > > - return zynqmp_pm_invoke_fn(PM_IOCTL, 0, > IOCTL_SET_BOOT_HEALTH_STATUS, > > - value, 0, NULL); > > + return zynqmp_pm_ioctl(0, IOCTL_SET_BOOT_HEALTH_STATUS, > value, > > + 0, NULL); > > } > > > > /** > > -- > > 2.7.4 > > > > This email and any attachments are intended for the sole use of the named > recipient(s) and contain(s) confidential information that may be proprietary, > privileged or copyrighted under applicable law. If you are not the intended > recipient, do not read, copy, or forward this email message or any > attachments. Delete this email message and any attachments immediately. > > Oops, this means I have to drop this, it's not compatible with kernel > development, sorry. > > greg k-h
I will incorporate your suggestions in the next version of patch-set. The footer message has unfortunately cropped up. Will also fix that in the next version.
Thanks, Amit
| |