[next] nouveau/gsp: replace zero-length array with flex-array member and use __counted_by

Message ID ZVZbX7C5suLMiBf+@work
State New
Headers
Series [next] nouveau/gsp: replace zero-length array with flex-array member and use __counted_by |

Commit Message

Gustavo A. R. Silva Nov. 16, 2023, 6:11 p.m. UTC
  Fake flexible arrays (zero-length and one-element arrays) are deprecated,
and should be replaced by flexible-array members. So, replace
zero-length array with a flexible-array member in `struct
PACKED_REGISTRY_TABLE`.

Also annotate array `entries` with `__counted_by()` to prepare for the
coming implementation by GCC and Clang of the `__counted_by` attribute.
Flexible array members annotated with `__counted_by` can have their
accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array
indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions).

This fixes multiple -Warray-bounds warnings:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1069:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1070:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1071:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1072:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]

While there, also make use of the struct_size() helper, and address
checkpatch.pl warning:
WARNING: please, no spaces at the start of a line

This results in no differences in binary output.

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 .../nvrm/535.113.01/nvidia/generated/g_os_nvoc.h   | 14 +++++++-------
 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c     |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)
  

Comments

Kees Cook Nov. 16, 2023, 6:28 p.m. UTC | #1
On Thu, Nov 16, 2023 at 12:11:43PM -0600, Gustavo A. R. Silva wrote:
> Fake flexible arrays (zero-length and one-element arrays) are deprecated,
> and should be replaced by flexible-array members. So, replace
> zero-length array with a flexible-array member in `struct
> PACKED_REGISTRY_TABLE`.
> 
> Also annotate array `entries` with `__counted_by()` to prepare for the
> coming implementation by GCC and Clang of the `__counted_by` attribute.
> Flexible array members annotated with `__counted_by` can have their
> accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array
> indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions).
> 
> This fixes multiple -Warray-bounds warnings:
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1069:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1070:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1071:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1072:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> 
> While there, also make use of the struct_size() helper, and address
> checkpatch.pl warning:
> WARNING: please, no spaces at the start of a line
> 
> This results in no differences in binary output.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Looks nice to me.

Reviewed-by: Kees Cook <keescook@chromium.org>
  
Danilo Krummrich Nov. 16, 2023, 7:45 p.m. UTC | #2
Hi Gustavo,

On Thu, Nov 16, 2023 at 12:11:43PM -0600, Gustavo A. R. Silva wrote:
> Fake flexible arrays (zero-length and one-element arrays) are deprecated,
> and should be replaced by flexible-array members. So, replace
> zero-length array with a flexible-array member in `struct
> PACKED_REGISTRY_TABLE`.
> 
> Also annotate array `entries` with `__counted_by()` to prepare for the
> coming implementation by GCC and Clang of the `__counted_by` attribute.
> Flexible array members annotated with `__counted_by` can have their
> accesses bounds-checked at run-time via `CONFIG_UBSAN_BOUNDS` (for array
> indexing) and `CONFIG_FORTIFY_SOURCE` (for strcpy/memcpy-family functions).
> 
> This fixes multiple -Warray-bounds warnings:
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1069:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1070:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1071:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1072:29: warning: array subscript 0 is outside array bounds of 'PACKED_REGISTRY_ENTRY[0]' [-Warray-bounds=]
> 
> While there, also make use of the struct_size() helper, and address
> checkpatch.pl warning:
> WARNING: please, no spaces at the start of a line
> 
> This results in no differences in binary output.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  .../nvrm/535.113.01/nvidia/generated/g_os_nvoc.h   | 14 +++++++-------
>  drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c     |  2 +-
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h b/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
> index 754c6af42f30..259b25c2ac6b 100644
> --- a/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
> +++ b/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
> @@ -28,17 +28,17 @@
>  
>  typedef struct PACKED_REGISTRY_ENTRY
>  {
> -    NvU32                   nameOffset;
> -    NvU8                    type;
> -    NvU32                   data;
> -    NvU32                   length;
> +	NvU32                   nameOffset;
> +	NvU8                    type;
> +	NvU32                   data;
> +	NvU32                   length;
>  } PACKED_REGISTRY_ENTRY;
>  
>  typedef struct PACKED_REGISTRY_TABLE
>  {
> -    NvU32                   size;
> -    NvU32                   numEntries;
> -    PACKED_REGISTRY_ENTRY   entries[0];
> +	NvU32                   size;
> +	NvU32                   numEntries;
> +	PACKED_REGISTRY_ENTRY   entries[] __counted_by(numEntries);
>  } PACKED_REGISTRY_TABLE;

Thanks for the fix!

However, I have some concerns about changing those header files, since they're
just copied over from Nvidia's driver [1].

Once we add the header files for a new firmware revision, we'd potentially run
into the same issue, applying the same fix again.

As I already mentioned for Timur's patch [2], I'd prefer to get a fix upstream
(meaning [1] in this case). Of course, that's probably more up to Timur to tell
if this will work out.

If we can't get a fix upstream, I'd probably prefer to silence warning
elsewhere.

[1] https://github.com/NVIDIA/open-gpu-kernel-modules
[2] https://lore.kernel.org/all/20231107234726.854248-1-ttabi@nvidia.com/T/

>  
>  #endif
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> index dc44f5c7833f..228335487af5 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
> @@ -1048,7 +1048,7 @@ r535_gsp_rpc_set_registry(struct nvkm_gsp *gsp)
>  	char *strings;
>  	int str_offset;
>  	int i;
> -	size_t rpc_size = sizeof(*rpc) + sizeof(rpc->entries[0]) * NV_GSP_REG_NUM_ENTRIES;
> +	size_t rpc_size = struct_size(rpc, entries, NV_GSP_REG_NUM_ENTRIES);
>  
>  	/* add strings + null terminator */
>  	for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++)
> -- 
> 2.34.1
>
  
Timur Tabi Nov. 16, 2023, 7:55 p.m. UTC | #3
On Thu, 2023-11-16 at 20:45 +0100, Danilo Krummrich wrote:
> As I already mentioned for Timur's patch [2], I'd prefer to get a fix
> upstream
> (meaning [1] in this case). Of course, that's probably more up to Timur to
> tell
> if this will work out.

Don't count on it.

Even if I did change [0] to [], I'm not going to be able to add the
"__counted_by(numEntries);" because that's just not something that our build
system uses.

And even then, I would need to change all [0] to [].  

You're not going to be able to use RM's header files as-is anyway in the
long term.  If we changed the layout of PACKED_REGISTRY_TABLE, we're not
going to create a PACKED_REGISTRY_TABLE2 and keep both around.  We're just
going to change PACKED_REGISTRY_TABLE and pretend the previous version never
existed.  You will then have to manually copy the new struct to your header
files and and maintain two versions yourself.
  
Danilo Krummrich Nov. 29, 2023, 1:01 a.m. UTC | #4
On 11/16/23 20:55, Timur Tabi wrote:
> On Thu, 2023-11-16 at 20:45 +0100, Danilo Krummrich wrote:
>> As I already mentioned for Timur's patch [2], I'd prefer to get a fix
>> upstream
>> (meaning [1] in this case). Of course, that's probably more up to Timur to
>> tell
>> if this will work out.
> 
> Don't count on it.

I see. Well, I think it's fine. Once we implement a decent abstraction we likely
don't need those header files in the kernel anymore.

@Gustavo, if you agree I will discard the indentation change when applying the
patch to keep the diff as small as possible.

- Danilo

> 
> Even if I did change [0] to [], I'm not going to be able to add the
> "__counted_by(numEntries);" because that's just not something that our build
> system uses.
> 
> And even then, I would need to change all [0] to [].
> 
> You're not going to be able to use RM's header files as-is anyway in the
> long term.  If we changed the layout of PACKED_REGISTRY_TABLE, we're not
> going to create a PACKED_REGISTRY_TABLE2 and keep both around.  We're just
> going to change PACKED_REGISTRY_TABLE and pretend the previous version never
> existed.  You will then have to manually copy the new struct to your header
> files and and maintain two versions yourself.
> 
> 
>
  
Gustavo A. R. Silva Nov. 29, 2023, 1:06 a.m. UTC | #5
On 11/28/23 19:01, Danilo Krummrich wrote:
> On 11/16/23 20:55, Timur Tabi wrote:
>> On Thu, 2023-11-16 at 20:45 +0100, Danilo Krummrich wrote:
>>> As I already mentioned for Timur's patch [2], I'd prefer to get a fix
>>> upstream
>>> (meaning [1] in this case). Of course, that's probably more up to Timur to
>>> tell
>>> if this will work out.
>>
>> Don't count on it.
> 
> I see. Well, I think it's fine. Once we implement a decent abstraction we likely
> don't need those header files in the kernel anymore.
> 
> @Gustavo, if you agree I will discard the indentation change when applying the
> patch to keep the diff as small as possible.

No problem.

Thanks
--
Gustavo
  
Danilo Krummrich Nov. 29, 2023, 2:15 a.m. UTC | #6
On 11/29/23 02:06, Gustavo A. R. Silva wrote:
> 
> 
> On 11/28/23 19:01, Danilo Krummrich wrote:
>> On 11/16/23 20:55, Timur Tabi wrote:
>>> On Thu, 2023-11-16 at 20:45 +0100, Danilo Krummrich wrote:
>>>> As I already mentioned for Timur's patch [2], I'd prefer to get a fix
>>>> upstream
>>>> (meaning [1] in this case). Of course, that's probably more up to Timur to
>>>> tell
>>>> if this will work out.
>>>
>>> Don't count on it.
>>
>> I see. Well, I think it's fine. Once we implement a decent abstraction we likely
>> don't need those header files in the kernel anymore.
>>
>> @Gustavo, if you agree I will discard the indentation change when applying the
>> patch to keep the diff as small as possible.
> 
> No problem.

Applied to drm-misc-fixes.

> 
> Thanks
> -- 
> Gustavo
> 
>
  

Patch

diff --git a/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h b/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
index 754c6af42f30..259b25c2ac6b 100644
--- a/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
+++ b/drivers/gpu/drm/nouveau/include/nvrm/535.113.01/nvidia/generated/g_os_nvoc.h
@@ -28,17 +28,17 @@ 
 
 typedef struct PACKED_REGISTRY_ENTRY
 {
-    NvU32                   nameOffset;
-    NvU8                    type;
-    NvU32                   data;
-    NvU32                   length;
+	NvU32                   nameOffset;
+	NvU8                    type;
+	NvU32                   data;
+	NvU32                   length;
 } PACKED_REGISTRY_ENTRY;
 
 typedef struct PACKED_REGISTRY_TABLE
 {
-    NvU32                   size;
-    NvU32                   numEntries;
-    PACKED_REGISTRY_ENTRY   entries[0];
+	NvU32                   size;
+	NvU32                   numEntries;
+	PACKED_REGISTRY_ENTRY   entries[] __counted_by(numEntries);
 } PACKED_REGISTRY_TABLE;
 
 #endif
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index dc44f5c7833f..228335487af5 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -1048,7 +1048,7 @@  r535_gsp_rpc_set_registry(struct nvkm_gsp *gsp)
 	char *strings;
 	int str_offset;
 	int i;
-	size_t rpc_size = sizeof(*rpc) + sizeof(rpc->entries[0]) * NV_GSP_REG_NUM_ENTRIES;
+	size_t rpc_size = struct_size(rpc, entries, NV_GSP_REG_NUM_ENTRIES);
 
 	/* add strings + null terminator */
 	for (i = 0; i < NV_GSP_REG_NUM_ENTRIES; i++)