Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190806073743.31575-1-oshpigelman@habana.ai> <20190806073743.31575-2-oshpigelman@habana.ai>
In-Reply-To: <20190806073743.31575-2-oshpigelman@habana.ai>
From:   Oded Gabbay <oded.gabbay@gmail.com>
Date:   Tue, 6 Aug 2019 15:22:35 +0300
Message-ID: <CAFCwf13UcH6=SY75HL7OSW_3GGpuNqP3dAUceE1eN5PHuAExOQ@mail.gmail.com>
Subject: Re: [PATCH 2/2] habanalabs: improve security in Debug IOCTL
To:     Omer Shpigelman <oshpigelman@habana.ai>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 6, 2019 at 10:38 AM Omer Shpigelman <oshpigelman@habana.ai> wrote:
>
> This patch improves the security in the Debug IOCTL.
> It adds checks that:
> - The register index value is in the allowed range for all opcodes.
> - The event types number is in the allowed range in SPMU enable.
> - The events number is in the allowed range in SPMU disable.
>
> Signed-off-by: Omer Shpigelman <oshpigelman@habana.ai>
> ---
>  drivers/misc/habanalabs/goya/goya_coresight.c | 72 ++++++++++++++++---
>  1 file changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/misc/habanalabs/goya/goya_coresight.c b/drivers/misc/habanalabs/goya/goya_coresight.c
> index d7ec7ad84cc6..4f7ffc137ab7 100644
> --- a/drivers/misc/habanalabs/goya/goya_coresight.c
> +++ b/drivers/misc/habanalabs/goya/goya_coresight.c
> @@ -15,6 +15,12 @@
>
>  #define GOYA_PLDM_CORESIGHT_TIMEOUT_USEC       (CORESIGHT_TIMEOUT_USEC * 100)
>
> +#define SPMU_SECTION_SIZE              DMA_CH_0_CS_SPMU_MAX_OFFSET
> +#define SPMU_EVENT_TYPES_OFFSET                0x400
> +#define SPMU_MAX_EVENT_TYPES           ((SPMU_SECTION_SIZE - \
> +                                               SPMU_EVENT_TYPES_OFFSET) / 4)
> +#define SPMU_MAX_EVENTS                        (SPMU_SECTION_SIZE / 4)
> +
>  static u64 debug_stm_regs[GOYA_STM_LAST + 1] = {
>         [GOYA_STM_CPU]          = mmCPU_STM_BASE,
>         [GOYA_STM_DMA_CH_0_CS]  = mmDMA_CH_0_CS_STM_BASE,
> @@ -226,9 +232,16 @@ static int goya_config_stm(struct hl_device *hdev,
>                 struct hl_debug_params *params)
>  {
>         struct hl_debug_params_stm *input;
> -       u64 base_reg = debug_stm_regs[params->reg_idx] - CFG_BASE;
> +       u64 base_reg;
>         int rc;
>
> +       if (params->reg_idx >= ARRAY_SIZE(debug_stm_regs)) {
> +               dev_err(hdev->dev, "Invalid register index in STM\n");
> +               return -EINVAL;
> +       }
> +
> +       base_reg = debug_stm_regs[params->reg_idx] - CFG_BASE;
> +
>         WREG32(base_reg + 0xFB0, CORESIGHT_UNLOCK);
>
>         if (params->enable) {
> @@ -288,10 +301,17 @@ static int goya_config_etf(struct hl_device *hdev,
>                 struct hl_debug_params *params)
>  {
>         struct hl_debug_params_etf *input;
> -       u64 base_reg = debug_etf_regs[params->reg_idx] - CFG_BASE;
> +       u64 base_reg;
>         u32 val;
>         int rc;
>
> +       if (params->reg_idx >= ARRAY_SIZE(debug_etf_regs)) {
> +               dev_err(hdev->dev, "Invalid register index in ETF\n");
> +               return -EINVAL;
> +       }
> +
> +       base_reg = debug_etf_regs[params->reg_idx] - CFG_BASE;
> +
>         WREG32(base_reg + 0xFB0, CORESIGHT_UNLOCK);
>
>         val = RREG32(base_reg + 0x304);
> @@ -445,11 +465,18 @@ static int goya_config_etr(struct hl_device *hdev,
>  static int goya_config_funnel(struct hl_device *hdev,
>                 struct hl_debug_params *params)
>  {
> -       WREG32(debug_funnel_regs[params->reg_idx] - CFG_BASE + 0xFB0,
> -                       CORESIGHT_UNLOCK);
> +       u64 base_reg;
> +
> +       if (params->reg_idx >= ARRAY_SIZE(debug_funnel_regs)) {
> +               dev_err(hdev->dev, "Invalid register index in FUNNEL\n");
> +               return -EINVAL;
> +       }
>
> -       WREG32(debug_funnel_regs[params->reg_idx] - CFG_BASE,
> -                       params->enable ? 0x33F : 0);
> +       base_reg = debug_funnel_regs[params->reg_idx] - CFG_BASE;
> +
> +       WREG32(base_reg + 0xFB0, CORESIGHT_UNLOCK);
> +
> +       WREG32(base_reg, params->enable ? 0x33F : 0);
>
>         return 0;
>  }
> @@ -458,9 +485,16 @@ static int goya_config_bmon(struct hl_device *hdev,
>                 struct hl_debug_params *params)
>  {
>         struct hl_debug_params_bmon *input;
> -       u64 base_reg = debug_bmon_regs[params->reg_idx] - CFG_BASE;
> +       u64 base_reg;
>         u32 pcie_base = 0;
>
> +       if (params->reg_idx >= ARRAY_SIZE(debug_bmon_regs)) {
> +               dev_err(hdev->dev, "Invalid register index in BMON\n");
> +               return -EINVAL;
> +       }
> +
> +       base_reg = debug_bmon_regs[params->reg_idx] - CFG_BASE;
> +
>         WREG32(base_reg + 0x104, 1);
>
>         if (params->enable) {
> @@ -522,7 +556,7 @@ static int goya_config_bmon(struct hl_device *hdev,
>  static int goya_config_spmu(struct hl_device *hdev,
>                 struct hl_debug_params *params)
>  {
> -       u64 base_reg = debug_spmu_regs[params->reg_idx] - CFG_BASE;
> +       u64 base_reg;
>         struct hl_debug_params_spmu *input = params->input;
>         u64 *output;
>         u32 output_arr_len;
> @@ -531,6 +565,13 @@ static int goya_config_spmu(struct hl_device *hdev,
>         u32 cycle_cnt_idx;
>         int i;
>
> +       if (params->reg_idx >= ARRAY_SIZE(debug_spmu_regs)) {
> +               dev_err(hdev->dev, "Invalid register index in SPMU\n");
> +               return -EINVAL;
> +       }
> +
> +       base_reg = debug_spmu_regs[params->reg_idx] - CFG_BASE;
> +
>         if (params->enable) {
>                 input = params->input;
>
> @@ -543,11 +584,18 @@ static int goya_config_spmu(struct hl_device *hdev,
>                         return -EINVAL;
>                 }
>
> +               if (input->event_types_num > SPMU_MAX_EVENT_TYPES) {
> +                       dev_err(hdev->dev,
> +                               "too many event types values for SPMU enable\n");
> +                       return -EINVAL;
> +               }
> +
>                 WREG32(base_reg + 0xE04, 0x41013046);
>                 WREG32(base_reg + 0xE04, 0x41013040);
>
>                 for (i = 0 ; i < input->event_types_num ; i++)
> -                       WREG32(base_reg + 0x400 + i * 4, input->event_types[i]);
> +                       WREG32(base_reg + SPMU_EVENT_TYPES_OFFSET + i * 4,
> +                               input->event_types[i]);
>
>                 WREG32(base_reg + 0xE04, 0x41013041);
>                 WREG32(base_reg + 0xC00, 0x8000003F);
> @@ -567,6 +615,12 @@ static int goya_config_spmu(struct hl_device *hdev,
>                         return -EINVAL;
>                 }
>
> +               if (events_num > SPMU_MAX_EVENTS) {
> +                       dev_err(hdev->dev,
> +                               "too many events values for SPMU disable\n");
> +                       return -EINVAL;
> +               }
> +
>                 WREG32(base_reg + 0xE04, 0x41013040);
>
>                 for (i = 0 ; i < events_num ; i++)
> --
> 2.17.1
>

Both patches are:
Reviewed-by: Oded Gabbay <oded.gabbay@gmail.com>