[PATCH v2 0/1] MR113: vkd3d-shader/dxbc: Validate and skip the signature section header size.

[Conor McCarthy (＠cmccarthy)]

Silences a very common warning.

--
  v2: vkd3d-shader/dxbc: Validate and skip the signature section header size.
 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113

[PATCH v2 1/1] vkd3d-shader/dxbc: Validate and skip the signature section header size.

[Conor McCarthy]

From: Conor McCarthy <cmccarthy(a)codeweavers.com>

Silences a very common warning.
---
 libs/vkd3d-shader/dxbc.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/libs/vkd3d-shader/dxbc.c b/libs/vkd3d-shader/dxbc.c
index d8eef8a5..1024b54e 100644
--- a/libs/vkd3d-shader/dxbc.c
+++ b/libs/vkd3d-shader/dxbc.c
@@ -1921,9 +1921,9 @@ static int shader_parse_signature(const struct vkd3d_shader_dxbc_section_desc *s
     bool has_stream_index, has_min_precision;
     struct vkd3d_shader_signature_element *e;
     const char *data = section->data.code;
+    uint32_t count, header_size;
     const char *ptr = data;
     unsigned int i;
-    uint32_t count;
 
     if (!require_space(0, 2, sizeof(uint32_t), section->data.size))
     {
@@ -1934,7 +1934,22 @@ static int shader_parse_signature(const struct vkd3d_shader_dxbc_section_desc *s
     read_dword(&ptr, &count);
     TRACE("%u elements.\n", count);
 
-    skip_dword_unknown(&ptr, 1); /* It seems to always be 0x00000008. */
+    read_dword(&ptr, &header_size);
+    if (header_size & (sizeof(uint32_t) - 1))
+    {
+        WARN("Header size %#x is not 32-bit aligned.\n", header_size);
+        return VKD3D_ERROR_INVALID_ARGUMENT;
+    }
+    header_size /= sizeof(uint32_t);
+    if (header_size < 2)
+    {
+        WARN("Invalid header size %u.\n", header_size);
+    }
+    else
+    {
+        for (header_size -= 2; header_size; --header_size)
+            skip_dword_unknown(&ptr, 1);
+    }
 
     if (!require_space(ptr - data, count, 6 * sizeof(uint32_t), section->data.size))
     {
-- 
GitLab

https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/dxbc.c:
> read_dword(&ptr, &count);
>      TRACE("%u elements.\n", count);
>
> -    skip_dword_unknown(&ptr, 1); /* It seems to always be 0x00000008. */
> +    read_dword(&ptr, &header_size);
> +    if (header_size & (sizeof(uint32_t) - 1))
> +    {
> +        WARN("Header size %#x is not 32-bit aligned.\n", header_size);
> +        return VKD3D_ERROR_INVALID_ARGUMENT;
> +    }
> +    header_size /= sizeof(uint32_t);
> +    if (header_size < 2)
> +    {
> +        WARN("Invalid header size %u.\n", header_size);
> +    }

Should we return an error here?

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113#note_26149

[Conor McCarthy (＠cmccarthy)]

On Fri Mar  3 23:19:30 2023 +0000, Zebediah Figura wrote:
> Should we return an error here?
> It certainly shouldn't happen in a valid shader. I see no harm in continuing as we've skipped the minimum of two dwords and it will error out soon enough if the the data is invalid.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113#note_26174

[Zebediah Figura (＠zfigura)]

On Mon Mar  6 01:10:50 2023 +0000, Conor McCarthy wrote:
> It certainly shouldn't happen in a valid shader. I see no harm in
> continuing as we've skipped the minimum of two dwords and it will error
> out soon enough if the the data is invalid.
> Sure, I just figure that we're also returning (or formally reporting an error) for other invalid shaders. Not that it seems like a big deal in any case.

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113#note_26175

[Henri Verbeet (＠hverbeet)]

> ```diff
> @@ -1934,7 +1934,22 @@ static int shader_parse_signature(const struct vkd3d_shader_dxbc_section_desc *s
>      read_dword(&ptr, &count);
>      TRACE("%u elements.\n", count);
>
> -    skip_dword_unknown(&ptr, 1); /* It seems to always be 0x00000008. */
> +    read_dword(&ptr, &header_size);
> +    if (header_size & (sizeof(uint32_t) - 1))
> +    {
> +        WARN("Header size %#x is not 32-bit aligned.\n", header_size);
> +        return VKD3D_ERROR_INVALID_ARGUMENT;
> +    }
> +    header_size /= sizeof(uint32_t);
> +    if (header_size < 2)
> +    {
> +        WARN("Invalid header size %u.\n", header_size);
> +    }
> +    else
> +    {
> +        for (header_size -= 2; header_size; --header_size)
> +            skip_dword_unknown(&ptr, 1);
> +    }
>
> if (!require_space(ptr - data, count, 6 * sizeof(uint32_t), section->data.size))
>      {
> ```

We should validate with require_space() that there is sufficient space left in the section to read the header.

When called from vkd3d_shader_parse_input_signature() in particular, ideally we'd use vkd3d_shader_error() to report parsing errors. This will require plumbing "message_context" from for_each_dxbc_section() through to "section_handler", and that's not an issue introduced in this patch, but since we're touching it...

-- 
https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/113#note_26213