drm/nouveau/secboot: remove VLA usage

PATCH In review Gustavo A. R. Silva 5 participants 7 messages Mar 13, 2018 – May 24, 2018

Subsystems (3)

DRM drivers DRM misc nouveau

lore

Share & tools

lore

Gustavo A. R. Silva

author

2018-03-13 16:24 UTC #0 lore raw

PATCH v2 1/1 drm/nouveau/secboot: remove VLA usage

In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.

The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.

Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.

 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c +3 -4

index 6f10b09..1e1f1c6 100644
@@ -80,12 +80,11 @@ acr_ls_msgqueue_post_run(struct nvkm_msgqueue *queue,
 			 struct nvkm_falcon *falcon, u32 addr_args)
 {
 	struct nvkm_device *device = falcon->owner->device;
-	u32 cmdline_size = NVKM_MSGQUEUE_CMDLINE_SIZE;
-	u8 buf[cmdline_size];
+	u8 buf[NVKM_MSGQUEUE_CMDLINE_SIZE];
 
-	memset(buf, 0, cmdline_size);
+	memset(buf, 0, sizeof(buf));
 	nvkm_msgqueue_write_cmdline(queue, buf);
-	nvkm_falcon_load_dmem(falcon, buf, addr_args, cmdline_size, 0);
+	nvkm_falcon_load_dmem(falcon, buf, addr_args, sizeof(buf), 0);
 	/* rearm the queue so it will wait for the init message */
 	nvkm_msgqueue_reinit(queue);

Signature · 1 line

2.7.4

Thierry Reding

Reviewed-by on v2 1/1

2018-03-14 11:08 UTC #1 lore raw

Quoted 14 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621

Quoted 8 lines

Signed-off-by Gustavo A. R. Silva <gustavo-L1vi/[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by Thierry Reding <[email protected]>

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Ben Skeggs

2018-03-16 02:05 UTC #2 lore raw

Quoted 21 lines · Thierry Reding

Quoted 20 lines · Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.

Quoted 4 lines

Quoted 2 lines
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by Thierry Reding <[email protected]>

Thanks everyone. I've taken the patch in my tree.

Ben.

Quoted 6 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Kees Cook

2018-04-26 23:25 UTC #3 lore raw

Quoted 22 lines · Ben Skeggs

Quoted 21 lines · Thierry Reding
Quoted 20 lines · Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.

Quoted 5 lines

Quoted 4 lines
Quoted 2 lines
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by Thierry Reding <[email protected]>
Thanks everyone. I've taken the patch in my tree.

Hi!

Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else?

Thanks!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Kees Cook

2018-05-23 22:48 UTC #4 lore raw

Quoted 26 lines · Kees Cook

Quoted 25 lines · Ben Skeggs
Quoted 24 lines · Thierry Reding
Quoted 22 lines · Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Quoted 7 lines

Quoted 2 lines
Quoted 1 line
Reviewed-by Thierry Reding <[email protected]>
Thanks everyone. I've taken the patch in my tree.
Hi!
Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else?

Hi, it's been another month; I still don't see this in linux-next. Daniel, can this go via drm-misc?

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Ben Skeggs

2018-05-24 00:36 UTC #5 lore raw

Quoted 30 lines · Kees Cook

Quoted 29 lines · Kees Cook
Quoted 27 lines · Ben Skeggs
Quoted 21 lines · Thierry Reding
Quoted 20 lines · Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.
Quoted 4 lines
Quoted 2 lines
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by Thierry Reding <[email protected]>
Thanks everyone. I've taken the patch in my tree.

Quoted 7 lines

Quoted 4 lines
Hi!
Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else?
Hi, it's been another month; I still don't see this in linux-next. Daniel, can this go via drm-misc?

It's already queued in drm-next.

Quoted 6 lines

-Kees
--
Kees Cook
Pixel Security

_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau

Kees Cook

2018-05-24 00:47 UTC #6 lore raw

Quoted 31 lines · Ben Skeggs

Quoted 30 lines · Kees Cook
Quoted 29 lines · Kees Cook
Quoted 27 lines · Ben Skeggs
Quoted 21 lines · Thierry Reding
Quoted 20 lines · Gustavo A. R. Silva
In preparation to enabling -Wvla, remove VLA. In this particular case directly use macro NVKM_MSGQUEUE_CMDLINE_SIZE instead of local variable cmdline_size. Also, remove cmdline_size as it is not actually useful anymore.
The use of stack Variable Length Arrays needs to be avoided, as they can be a vector for stack exhaustion, which can be both a runtime bug or a security flaw. Also, in general, as code evolves it is easy to lose track of how big a VLA can get. Thus, we can end up having runtime failures that are hard to debug.
Also, fixed as part of the directive to remove all VLAs from the kernel: https://lkml.org/lkml/2018/3/7/621
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
Changes in v2:
- Use sizeof(buf) instead of NVKM_MSGQUEUE_CMDLINE_SIZE. This change is based on the feedback provided by David Laight. Thanks David.
Quoted 4 lines
Quoted 2 lines
 drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by Thierry Reding <[email protected]>
Thanks everyone. I've taken the patch in my tree.

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
Hi!
Just checking in on this -- I don't see this patch in linux-next. Is this queued somewhere else?
Hi, it's been another month; I still don't see this in linux-next. Daniel, can this go via drm-misc?
It's already queued in drm-next.

Ah-ha, great, thanks! Looks like I just got unlucky with linux-next pausing on the 17th and this getting committed on the 18th. :) But, yes, I see it now:
https://cgit.freedesktop.org/drm/drm/commit/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/ls_ucode_msgqueue.c?id=7bf5b70befd7817b9e42acbd2291b2042ea1bf81

-Kees

Signature · 6 lines

Kees Cook
Pixel Security
_______________________________________________
Nouveau mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/nouveau