[PATCH 0/2] Fixes in drm/i915/cmdparser.

[PATCH 0/2] Fixes in drm/i915/cmdparser.

[Michal Srb]

Hi,

I have tried to extract the intel_engine_cmd_parser into a user-space binary
and run libFuzzer on it. It found two ways to cause undefined behavior.

I am not completely sure if the same issues can be triggered in the driver, or
if something would prevent them from happening. Still I thought it is worth
sharing it here.

Michal Srb (2):
  drm/i915/cmdparser: Check reg_table_count before derefencing.
  drm/i915/cmdparser: Do not check bits past the cmd length.

 drivers/gpu/drm/i915/i915_cmd_parser.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/i915/cmdparser: Check reg_table_count before derefencing.

[Michal Srb]

The find_reg function was assuming that there is always at least one table in
reg_tables. It is not always true.

In case of VCS or VECS, the reg_tables is NULL and reg_table_count is 0,
implying that no register-accessing commands are allowed. However, the command
tables include commands such as MI_STORE_REGISTER_MEM. When trying to check
such command, the find_reg would dereference NULL pointer.

Now it will just return NULL meaning that the register was not found and the
command will be rejected.

Signed-off-by: Michal Srb <msrb@suse.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 8ba932b22f7c..de7ec59433d1 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1038,7 +1038,7 @@ find_reg(const struct intel_engine_cs *engine, bool is_master, u32 addr)
 	const struct drm_i915_reg_table *table = engine->reg_tables;
 	int count = engine->reg_table_count;
 
-	do {
+	for (; count > 0; ++table, --count) {
 		if (!table->master || is_master) {
 			const struct drm_i915_reg_descriptor *reg;
 
@@ -1046,7 +1046,7 @@ find_reg(const struct intel_engine_cs *engine, bool is_master, u32 addr)
 			if (reg != NULL)
 				return reg;
 		}
-	} while (table++, --count);
+	}
 
 	return NULL;
 }
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/i915/cmdparser: Do not check past the cmd length.

[Michal Srb]

The command MEDIA_VFE_STATE checks bits at offset +2 dwords. However, it is
possible to have MEDIA_VFE_STATE command with length = 0 + LENGTH_BIAS = 2.
In that case check_cmd will read bits from the following command, or even past
the end of the buffer.

Similarly to how registers are checked - if the offset ends up outside of the
command length, just ignore it.

Signed-off-by: Michal Srb <msrb@suse.com>
---
 drivers/gpu/drm/i915/i915_cmd_parser.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index de7ec59433d1..827740b866a8 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1218,6 +1218,9 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 					continue;
 			}
 
+			if (desc->bits[i].offset >= length)
+				continue;
+
 			dword = cmd[desc->bits[i].offset] &
 				desc->bits[i].mask;
 
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/i915/cmdparser: Do not check past the cmd length.

[Chris Wilson]

Quoting Michal Srb (2018-02-05 14:29:16)
> The command MEDIA_VFE_STATE checks bits at offset +2 dwords. However, it is
> possible to have MEDIA_VFE_STATE command with length = 0 + LENGTH_BIAS = 2.
> In that case check_cmd will read bits from the following command, or even past
> the end of the buffer.
> 
> Similarly to how registers are checked - if the offset ends up outside of the
> command length, just ignore it.
> 
> Signed-off-by: Michal Srb <msrb@suse.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index de7ec59433d1..827740b866a8 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1218,6 +1218,9 @@ static bool check_cmd(const struct intel_engine_cs *engine,
>                                         continue;
>                         }
>  
> +                       if (desc->bits[i].offset >= length)
> +                               continue;

Should be return false since the command can't be validated.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm/i915/cmdparser: Do not check past the cmd length.

[Michal Srb]

The command MEDIA_VFE_STATE checks bits at offset +2 dwords. However, it is
possible to have MEDIA_VFE_STATE command with length = 0 + LENGTH_BIAS = 2.
In that case check_cmd will read bits from the following command, or even past
the end of the buffer.

If the offset ends up outside of the command length, reject the command.

Signed-off-by: Michal Srb <msrb@suse.com>
---

 v2: Return false instead of continuing - reject the command instead of
     ignoring.

 drivers/gpu/drm/i915/i915_cmd_parser.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index de7ec59433d1..ef7ad016d67c 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -1218,6 +1218,12 @@ static bool check_cmd(const struct intel_engine_cs *engine,
 					continue;
 			}
 
+			if (desc->bits[i].offset >= length) {
+				DRM_DEBUG_DRIVER("CMD: Rejected command 0x%08X, too short to check bitmask (%s)\n",
+						 *cmd, engine->name);
+				return false;
+			}
+
 			dword = cmd[desc->bits[i].offset] &
 				desc->bits[i].mask;
 
-- 
2.13.6

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/i915/cmdparser: Do not check past the cmd length.

[Chris Wilson]

Quoting Michal Srb (2018-02-05 15:17:45)
> The command MEDIA_VFE_STATE checks bits at offset +2 dwords. However, it is
> possible to have MEDIA_VFE_STATE command with length = 0 + LENGTH_BIAS = 2.
> In that case check_cmd will read bits from the following command, or even past
> the end of the buffer.
> 
> If the offset ends up outside of the command length, reject the command.
> 
> Signed-off-by: Michal Srb <msrb@suse.com>

Looks good, both
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I'll resend them to intel-gfx@ so CI picks them up for the checklist.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm/i915/cmdparser: Do not check past the cmd length.

[Chris Wilson]

Quoting Chris Wilson (2018-02-05 16:04:25)
> Quoting Michal Srb (2018-02-05 15:17:45)
> > The command MEDIA_VFE_STATE checks bits at offset +2 dwords. However, it is
> > possible to have MEDIA_VFE_STATE command with length = 0 + LENGTH_BIAS = 2.
> > In that case check_cmd will read bits from the following command, or even past
> > the end of the buffer.
> > 
> > If the offset ends up outside of the command length, reject the command.
> > 
> > Signed-off-by: Michal Srb <msrb@suse.com>
> 
> Looks good, both
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I'll resend them to intel-gfx@ so CI picks them up for the checklist.

Added

Fixes: 76ff480ec963 ("drm/i915/cmdparser: Use binary search for faster
register lookup"

and

Fixes: 351e3db2b363 ("drm/i915: Implement command buffer parsing logic")

respectively and pushed. Thanks for the patches,
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel