diff mbox series

[4/4] drm/i915/guc: Verify hwconfig blob matches supported format

Message ID 20220207192854.862959-5-jordan.l.justen@intel.com (mailing list archive)
State New, archived
Headers show
Series GuC HWCONFIG with documentation | expand

Commit Message

Jordan Justen Feb. 7, 2022, 7:28 p.m. UTC 
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

Comments

kernel test robot Feb. 7, 2022, 10:45 p.m. UTC | #1 
Hi Jordan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-m021-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080601.vp3JG4Dh-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16fb566
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
        git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c: In function 'verify_hwconfig_blob':
>> drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
      79 |  struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
         |  ^~~~~~


vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

    73	
    74	static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
    75	{
    76		if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
    77			return -EINVAL;
    78	
  > 79		struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
    80		u32 remaining = (hwconfig->size / 4);
    81		while (remaining > 0) {
    82			if (remaining < 2)
    83				return -EINVAL;
    84			if (pos->length > remaining - 2)
    85				return -EINVAL;
    86			remaining -= 2 + pos->length;
    87			pos = (void *)&pos->data[pos->length];
    88		}
    89	
    90		DRM_INFO("hwconfig blob format appears valid\n");
    91		return 0;
    92	}
    93	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 7, 2022, 10:45 p.m. UTC | #2 
Hi Jordan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207]
[cannot apply to airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-buildonly-randconfig-r001-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080648.8LUbxyNz-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0d8850ae2cae85d49bea6ae0799fa41c7202c05c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16fb566
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
        git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:44: warning: mixing declarations and code is a C99 extension [-Wdeclaration-after-statement]
           struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
                                                     ^
   1 warning generated.


vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

    73	
    74	static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
    75	{
    76		if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
    77			return -EINVAL;
    78	
  > 79		struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
    80		u32 remaining = (hwconfig->size / 4);
    81		while (remaining > 0) {
    82			if (remaining < 2)
    83				return -EINVAL;
    84			if (pos->length > remaining - 2)
    85				return -EINVAL;
    86			remaining -= 2 + pos->length;
    87			pos = (void *)&pos->data[pos->length];
    88		}
    89	
    90		DRM_INFO("hwconfig blob format appears valid\n");
    91		return 0;
    92	}
    93	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
kernel test robot Feb. 7, 2022, 11:36 p.m. UTC | #3 
Hi Jordan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip drm-exynos/exynos-drm-next drm/drm-next tegra-drm/drm/tegra/for-next v5.17-rc3 next-20220207]
[cannot apply to airlied/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a016-20220207 (https://download.01.org/0day-ci/archive/20220208/202202080749.RRjxhCB2-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/c122b0dea958e76766ce9b4b9f34d2eed16fb566
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jordan-Justen/GuC-HWCONFIG-with-documentation/20220208-032950
        git checkout c122b0dea958e76766ce9b4b9f34d2eed16fb566
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c: In function 'verify_hwconfig_blob':
>> drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c:79:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
      79 |  struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
         |  ^~~~~~
   cc1: all warnings being treated as errors


vim +79 drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c

    73	
    74	static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
    75	{
    76		if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
    77			return -EINVAL;
    78	
  > 79		struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
    80		u32 remaining = (hwconfig->size / 4);
    81		while (remaining > 0) {
    82			if (remaining < 2)
    83				return -EINVAL;
    84			if (pos->length > remaining - 2)
    85				return -EINVAL;
    86			remaining -= 2 + pos->length;
    87			pos = (void *)&pos->data[pos->length];
    88		}
    89	
    90		DRM_INFO("hwconfig blob format appears valid\n");
    91		return 0;
    92	}
    93	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Tvrtko Ursulin Feb. 8, 2022, 9:36 a.m. UTC | #4 
Hi,

Commit message please.

Will GuC folks be reviewing this work?

Quick sanity check maybe makes sense, given data is being "sent" to 
userspace directly, I am just not sure if it is worth having in 
non-debug builds of i915. Though I will agree not having it in 
production then largely defeats the purpose so dunno. Effective 
difference if GuC load fails versus userspace libraries failing to parse 
hwconfig?

On 07/02/2022 19:28, Jordan Justen wrote:
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>   .../gpu/drm/i915/gt/uc/intel_guc_hwconfig.c   | 26 +++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> index ce6088f112d4..695ef7a8f519 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
> @@ -71,6 +71,26 @@ static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig)
>   	return 0;
>   }
>   
> +static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
> +{
> +	if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
> +		return -EINVAL;

So individual item size is minimum one u32, or zero? Document that in 
patch 3?

> +
> +	struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;

kbuild robot told you about mixing declarations and code already. :)

> +	u32 remaining = (hwconfig->size / 4);

Blank line here and braces not needed.

> +	while (remaining > 0) {
> +		if (remaining < 2)
> +			return -EINVAL;
> +		if (pos->length > remaining - 2)
> +			return -EINVAL;
> +		remaining -= 2 + pos->length;
> +		pos = (void *)&pos->data[pos->length];
> +	}
> +
> +	DRM_INFO("hwconfig blob format appears valid\n");

Probably debug level at most.

> +	return 0;
> +}
> +
>   static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   {
>   	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
> @@ -91,6 +111,12 @@ static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
>   	if (ret >= 0)
>   		memcpy(hwconfig->ptr, vaddr, hwconfig->size);
>   
> +	if (verify_hwconfig_blob(hwconfig)) {

Merge under the "ret >= 0" branch above?

> +		DRM_ERROR("Ignoring invalid hwconfig blob received from "
> +			  "GuC!\n");

Use drm_dbg/drm_err so the log is tied to a device in multi-gpu systems.

Also keep the string on one line as per kernel coding style guide.

Regards,

Tvrtko

> +		return -EINVAL;
> +	}
> +
>   	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
>   
>   	return ret;
Jordan Justen Feb. 8, 2022, 10:45 p.m. UTC | #5 
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes:

> Will GuC folks be reviewing this work?

I don't know. If you mean the i915 devs interfacing with the GuC, I know
John/Rodrigo seemed a bit resistant writing the patches to give
userspace this trivial format guarantee on the blob.

So, I hoped they would write the patches (3 & 4 in my series), but now
I'm hoping they will at least accept the patches.

> Quick sanity check maybe makes sense, given data is being "sent" to 
> userspace directly, I am just not sure if it is worth having in 
> non-debug builds of i915. Though I will agree not having it in 
> production then largely defeats the purpose so dunno.

The check seems fairly trivial, and it seems like i915 should provide
whatever guidance/guarantee is possible to userspace. (Thus, do it once
per boot even on release builds.) See also, the commit message I added.

> Effective difference if GuC load fails versus userspace libraries
> failing to parse hwconfig?

Lots of potential things to consider. Personally, I think for internal
Intel builds, if this check fails, then it ought to cause the GuC to
always fail to load, which today means the device will be wedged.

For external builds, I think it should still load the GuC but not send
the blob to userspace. This is what should happen with the patches in
this series. (I really hope this never happens, which it why I think the
internal builds should be so harsh.)

Now ... if i915 ever regains the ability to drive the device without the
closed source GuC, well... No reason to go off on unrealistic tangents. :)

Also, later down the road for released products, userspace drivers may
choose to bypass the hwconfig to limit the dependence on GuC. That is a
related, but different topic.

-Jordan
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
index ce6088f112d4..695ef7a8f519 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_hwconfig.c
@@ -71,6 +71,26 @@  static int guc_hwconfig_discover_size(struct intel_guc_hwconfig *hwconfig)
 	return 0;
 }
 
+static int verify_hwconfig_blob(const struct intel_guc_hwconfig *hwconfig)
+{
+	if (hwconfig->size % 4 != 0 || hwconfig->ptr == NULL)
+		return -EINVAL;
+
+	struct drm_i915_query_hwconfig_blob_item *pos = hwconfig->ptr;
+	u32 remaining = (hwconfig->size / 4);
+	while (remaining > 0) {
+		if (remaining < 2)
+			return -EINVAL;
+		if (pos->length > remaining - 2)
+			return -EINVAL;
+		remaining -= 2 + pos->length;
+		pos = (void *)&pos->data[pos->length];
+	}
+
+	DRM_INFO("hwconfig blob format appears valid\n");
+	return 0;
+}
+
 static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 {
 	struct intel_guc *guc = hwconfig_to_guc(hwconfig);
@@ -91,6 +111,12 @@  static int guc_hwconfig_fill_buffer(struct intel_guc_hwconfig *hwconfig)
 	if (ret >= 0)
 		memcpy(hwconfig->ptr, vaddr, hwconfig->size);
 
+	if (verify_hwconfig_blob(hwconfig)) {
+		DRM_ERROR("Ignoring invalid hwconfig blob received from "
+			  "GuC!\n");
+		return -EINVAL;
+	}
+
 	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
 
 	return ret;