Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Amit Sunil Dhamne <amitsuni@xilinx.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
        "mingo@kernel.org" <mingo@kernel.org>,
        "matt@codeblueprint.co.uk" <matt@codeblueprint.co.uk>,
        "sudeep.holla@arm.com" <sudeep.holla@arm.com>,
        "hkallweit1@gmail.com" <hkallweit1@gmail.com>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "dmitry.torokhov@gmail.com" <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-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.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
Thread-Topic: [PATCH 1/3] firmware: xilinx: Add validation check for IOCTL
Thread-Index: AQHWhwg4A4eef5X5ukS95py8Uwip4KlrLFsAgABXakA=
Date:   Wed, 16 Sep 2020 16:53:24 +0000
Message-ID: <BYAPR02MB56225CBE23F9F9CCE157F63FA7210@BYAPR02MB5622.namprd02.prod.outlook.com>
References: <1599697204-32103-1-git-send-email-amit.sunil.dhamne@xilinx.com>
 <1599697204-32103-2-git-send-email-amit.sunil.dhamne@xilinx.com>
 <20200916113334.GB1124534@kroah.com>
In-Reply-To: <20200916113334.GB1124534@kroah.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 41be25ff-ce9f-48fc-8662-08d85a61073b
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Sep 2020 16:53:25.0385
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: kqsa6GXpD6O9F1x8VAcCbFJ06d1JxpKz+OwE+maCnNMBZKne7IOckcrLBUTKS8WDO70CRkpa7HquHTx0E1e+0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR02MB6852
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>=20
> 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;
>=20
> bool is nicer, right?
>=20
> > +       }
> > +}
> > +
> > +/**
> > + * 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 =3D of_find_compatible_node(NULL, NULL, "xlnx,versal");
> > +       if (np) {
> > +               if (!versal_is_valid_ioctl(ioctl_id))
> > +                       return -EINVAL;
>=20
> Wrong error value.
>=20
> > +       } else {
> > +               if (!zynqmp_is_valid_ioctl(ioctl_id))
> > +                       return -EINVAL;
>=20
> Wrong error value.
>=20
> > +       }
> > +       of_node_put(np);
> > +
> > +       return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id, arg1,
> arg2,
> > +                                  out);
>=20
> No other checking of ioctl commands and arguments?  Brave...
>=20
> > +}
> > +
> > +/**
> >   * 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 nam=
ed
> recipient(s) and contain(s) confidential information that may be propriet=
ary,
> privileged or copyrighted under applicable law. If you are not the intend=
ed
> recipient, do not read, copy, or forward this email message or any
> attachments. Delete this email message and any attachments immediately.
>=20
> Oops, this means I have to drop this, it's not compatible with kernel
> development, sorry.
>=20
> 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