* [PATCH 1/2] habanalabs: use default structure for user input in Debug IOCTL
@ 2019-08-06 7:37 Omer Shpigelman
2019-08-06 7:37 ` [PATCH 2/2] habanalabs: improve security " Omer Shpigelman
0 siblings, 1 reply; 3+ messages in thread
From: Omer Shpigelman @ 2019-08-06 7:37 UTC (permalink / raw)
To: oded.gabbay; +Cc: linux-kernel
This patch fixes a possible kernel crash when a user provides a too small
input structure to the Debug IOCTL.
The fix sets a default input structure and copies to it the user data.
In case the user provided as input a too small structure, the code will
use the default values taken from the default structure.
Note that in contrary to the input structure, the user can provide an
output structure with changing size or no size at all. Therefore the user
output structure validation is already done in the Debug logic later on.
Signed-off-by: Omer Shpigelman <oshpigelman@habana.ai>
---
drivers/misc/habanalabs/habanalabs_ioctl.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/habanalabs/habanalabs_ioctl.c b/drivers/misc/habanalabs/habanalabs_ioctl.c
index ce0cd93a8421..3ce65459b01c 100644
--- a/drivers/misc/habanalabs/habanalabs_ioctl.c
+++ b/drivers/misc/habanalabs/habanalabs_ioctl.c
@@ -144,13 +144,16 @@ static int debug_coresight(struct hl_device *hdev, struct hl_debug_args *args)
params->op = args->op;
if (args->input_ptr && args->input_size) {
- input = memdup_user(u64_to_user_ptr(args->input_ptr),
- args->input_size);
- if (IS_ERR(input)) {
- rc = PTR_ERR(input);
- input = NULL;
- dev_err(hdev->dev,
- "error %d when copying input debug data\n", rc);
+ input = kzalloc(hl_debug_struct_size[args->op], GFP_KERNEL);
+ if (!input) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ if (copy_from_user(input, u64_to_user_ptr(args->input_ptr),
+ args->input_size)) {
+ rc = -EFAULT;
+ dev_err(hdev->dev, "failed to copy input debug data\n");
goto out;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH 2/2] habanalabs: improve security in Debug IOCTL
2019-08-06 7:37 [PATCH 1/2] habanalabs: use default structure for user input in Debug IOCTL Omer Shpigelman
@ 2019-08-06 7:37 ` Omer Shpigelman
2019-08-06 12:22 ` Oded Gabbay
0 siblings, 1 reply; 3+ messages in thread
From: Omer Shpigelman @ 2019-08-06 7:37 UTC (permalink / raw)
To: oded.gabbay; +Cc: linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] habanalabs: improve security in Debug IOCTL
2019-08-06 7:37 ` [PATCH 2/2] habanalabs: improve security " Omer Shpigelman
@ 2019-08-06 12:22 ` Oded Gabbay
0 siblings, 0 replies; 3+ messages in thread
From: Oded Gabbay @ 2019-08-06 12:22 UTC (permalink / raw)
To: Omer Shpigelman; +Cc: linux-kernel
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-06 12:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 7:37 [PATCH 1/2] habanalabs: use default structure for user input in Debug IOCTL Omer Shpigelman
2019-08-06 7:37 ` [PATCH 2/2] habanalabs: improve security " Omer Shpigelman
2019-08-06 12:22 ` Oded Gabbay
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).