[PATCH] drm/nouveau: Removes unnecessary args check in nouveau_uvmm_sm_prepare
Checking `args` after calling `op_map_prepare` is unnecessary since
if `op_map_prepare` was to be called with NULL args, it would lead
to a NULL pointer dereference, thus never hitting that check.
Hence this check can be removed, and a note added to remind users of
this function to ensure that args != NULL when calling this function
for a map operation as it was suggested by Danilo [1]
[1] https://lore.kernel.org/lkml/6a1ebcef-bade-45a0-9bd9-c05f0226e...@redhat.com
Suggested-by: Danilo Krummrich
Signed-off-by: Yuran Pereira
---
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index 5cf892c50f43..c8c3f1b1b604 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -604,6 +604,10 @@ op_unmap_prepare(struct drm_gpuva_op_unmap *u)
drm_gpuva_unmap(u);
}
+/*
+ * Note: @args should not be NULL when calling for
+ * a map operation.
+ */
static int
nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
struct nouveau_uvma_prealloc *new,
@@ -624,7 +628,7 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
if (ret)
goto unwind;
- if (args && vmm_get_range) {
+ if (vmm_get_range) {
ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start,
vmm_get_range);
if (ret) {
--
2.25.1
Re: [PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare
Hello Danilo, On Tue, Nov 14, 2023 at 05:23:59PM +0100, Danilo Krummrich wrote: > Hi Yuran, > > op_map_prepare() can't be called with `args` being NULL, since when called > through nouveau_uvmm_sm_unmap_prepare() we can't hit the DRM_GPUVA_OP_MAP > case at all. > > Unmapping something never leads to a new mapping being created, it can lead > to remaps though. > Yes, you're right. I certainly hadn't noticed that when I first submitted this patch. > > This check is not required for the reason given above. If you like, you > can change this patch up to remove the args check and add a comment like: > > /* args can't be NULL when called for a map operation. */ > Sure, I'll do that, sounds reasonable. Thank you for your feedback. Yuran > > Yeah, I see how this unnecessary check made you think so. > > - Danilo > >
Re: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in amdgpu_dm_setup_replay
Hello,
On Fri, Oct 27, 2023 at 11:57:45AM -0400, Hamza Mahfooz wrote:
> On 10/27/23 11:55, Lakha, Bhawanpreet wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> > There was a consensus to use memset instead of {0}. I remember making
> > changes related to that previously.
>
> Hm, seems like it's used rather consistently in the DM and in DC
> though.
>
Have you decided which one should be used?
Should I submit a v2 patch using {0} instead of memset?
Yuran Pereira
> >
> > Bhawan
> >
> >
> > *From:* Mahfooz, Hamza
> > *Sent:* October 27, 2023 11:53 AM
> > *To:* Yuran Pereira ; airl...@gmail.com
> >
> > *Cc:* Li, Sun peng (Leo) ; Lakha, Bhawanpreet
> > ; Pan, Xinhui ; Siqueira,
> > Rodrigo ; linux-ker...@vger.kernel.org
> > ; amd-...@lists.freedesktop.org
> > ; dri-devel@lists.freedesktop.org
> > ; Deucher, Alexander
> > ; Koenig, Christian
> > ;
> > linux-kernel-ment...@lists.linuxfoundation.org
> >
> > *Subject:* Re: [PATCH] drm/amdgpu: Fixes uninitialized variable usage in
> > amdgpu_dm_setup_replay
> > On 10/26/23 17:25, Yuran Pereira wrote:
> > > Since `pr_config` is not initialized after its declaration, the
> > > following operations with `replay_enable_option` may be performed
> > > when `replay_enable_option` is holding junk values which could
> > > possibly lead to undefined behaviour
> > >
> > > ```
> > > ...
> > > pr_config.replay_enable_option |= pr_enable_option_static_screen;
> > > ...
> > >
> > > if (!pr_config.replay_timing_sync_supported)
> > > pr_config.replay_enable_option &= ~pr_enable_option_general_ui;
> > > ...
> > > ```
> > >
> > > This patch initializes `pr_config` after its declaration to ensure that
> > > it doesn't contain junk data, and prevent any undefined behaviour
> > >
> > > Addresses-Coverity-ID: 1544428 ("Uninitialized scalar variable")
> > > Fixes: dede1fea4460 ("drm/amd/display: Add Freesync Panel DM code")
> > > Signed-off-by: Yuran Pereira
> > > ---
> > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> > > index 32d3086c4cb7..40526507f50b 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
> > > @@ -23,6 +23,7 @@
> > > *
> > > */
> > > +#include
> > > #include "amdgpu_dm_replay.h"
> > > #include "dc.h"
> > > #include "dm_helpers.h"
> > > @@ -74,6 +75,8 @@ bool amdgpu_dm_setup_replay(struct dc_link *link,
> > > struct amdgpu_dm_connector *ac
> > > struct replay_config pr_config;
> >
> > I would prefer setting pr_config = {0};
> >
> > > union replay_debug_flags *debug_flags = NULL;
> > > + memset(_config, 0, sizeof(pr_config));
> > > +
> > > // For eDP, if Replay is supported, return true to skip checks
> > > if (link->replay_settings.config.replay_supported)
> > > return true;
> > --
> > Hamza
> >
> --
> Hamza
>
[PATCH] drm/amdgpu: Fixes uninitialized variable usage in amdgpu_dm_setup_replay
Since `pr_config` is not initialized after its declaration, the
following operations with `replay_enable_option` may be performed
when `replay_enable_option` is holding junk values which could
possibly lead to undefined behaviour
```
...
pr_config.replay_enable_option |= pr_enable_option_static_screen;
...
if (!pr_config.replay_timing_sync_supported)
pr_config.replay_enable_option &= ~pr_enable_option_general_ui;
...
```
This patch initializes `pr_config` after its declaration to ensure that
it doesn't contain junk data, and prevent any undefined behaviour
Addresses-Coverity-ID: 1544428 ("Uninitialized scalar variable")
Fixes: dede1fea4460 ("drm/amd/display: Add Freesync Panel DM code")
Signed-off-by: Yuran Pereira
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
index 32d3086c4cb7..40526507f50b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_replay.c
@@ -23,6 +23,7 @@
*
*/
+#include
#include "amdgpu_dm_replay.h"
#include "dc.h"
#include "dm_helpers.h"
@@ -74,6 +75,8 @@ bool amdgpu_dm_setup_replay(struct dc_link *link, struct
amdgpu_dm_connector *ac
struct replay_config pr_config;
union replay_debug_flags *debug_flags = NULL;
+ memset(_config, 0, sizeof(pr_config));
+
// For eDP, if Replay is supported, return true to skip checks
if (link->replay_settings.config.replay_supported)
return true;
--
2.25.1
[PATCH] drm/nouveau: Prevents NULL pointer dereference in nouveau_uvmm_sm_prepare
There are instances where the "args" argument passed to
nouveau_uvmm_sm_prepare() is NULL.
I.e. when nouveau_uvmm_sm_prepare() is called from
nouveau_uvmm_sm_unmap_prepare()
```
static int
nouveau_uvmm_sm_unmap_prepare(struct nouveau_uvmm *uvmm,
...
{
return nouveau_uvmm_sm_prepare(uvmm, new, ops, NULL);
}
```
The problem is that op_map_prepare() which nouveau_uvmm_sm_prepare
calls, dereferences this value, which can lead to a NULL pointer
dereference.
```
static int
op_map_prepare(struct nouveau_uvmm *uvmm,
...
{
...
uvma->region = args->region; <-- Dereferencing of possibly NULL pointer
uvma->kind = args->kind; <--
...
}
```
```
static int
nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
...
struct uvmm_map_args *args)
{
struct drm_gpuva_op *op;
u64 vmm_get_start = args ? args->addr : 0;
u64 vmm_get_end = args ? args->addr + args->range : 0;
int ret;
drm_gpuva_for_each_op(op, ops) {
switch (op->op) {
case DRM_GPUVA_OP_MAP: {
u64 vmm_get_range = vmm_get_end - vmm_get_start;
ret = op_map_prepare(uvmm, >map, >map, args); <---
if (ret)
goto unwind;
if (args && vmm_get_range) {
ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start,
vmm_get_range);
if (ret) {
op_map_prepare_unwind(new->map);
goto unwind;
}
}
...
```
Since the switch "case DRM_GPUVA_OP_MAP", also NULL checks "args"
after the call to op_map_prepare(), my guess is that we should
probably relocate this check to a point before op_map_prepare()
is called.
This patch ensures that the value of args is checked before
calling op_map_prepare()
Addresses-Coverity-ID: 1544574 ("Dereference after null check")
Signed-off-by: Yuran Pereira
---
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index aae780e4a4aa..6baa481eb2c8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -620,11 +620,14 @@ nouveau_uvmm_sm_prepare(struct nouveau_uvmm *uvmm,
case DRM_GPUVA_OP_MAP: {
u64 vmm_get_range = vmm_get_end - vmm_get_start;
+ if (!args)
+ goto unwind;
+
ret = op_map_prepare(uvmm, >map, >map, args);
if (ret)
goto unwind;
- if (args && vmm_get_range) {
+ if (vmm_get_range) {
ret = nouveau_uvmm_vmm_get(uvmm, vmm_get_start,
vmm_get_range);
if (ret) {
--
2.25.1
[PATCH] drm: docs: Remove item from TODO list
Since "Clean up checks for already prepared/enabled in panels" has
already been done and merged [1], I think there is no longer a need
for this item to be in the gpu TODO.
[1] https://patchwork.freedesktop.org/patch/551421/
Signed-off-by: Yuran Pereira
---
Documentation/gpu/todo.rst | 25 -
1 file changed, 25 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 03fe5d1247be..280020b0ad4d 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -460,31 +460,6 @@ Contact: Thomas Zimmermann
Level: Starter
-Clean up checks for already prepared/enabled in panels
---
-
-In a whole pile of panel drivers, we have code to make the
-prepare/unprepare/enable/disable callbacks behave as no-ops if they've already
-been called. To get some idea of the duplicated code, try::
-
- git grep 'if.*>prepared' -- drivers/gpu/drm/panel
- git grep 'if.*>enabled' -- drivers/gpu/drm/panel
-
-In the patch ("drm/panel: Check for already prepared/enabled in drm_panel")
-we've moved this check to the core. Now we can most definitely remove the
-check from the individual panels and save a pile of code.
-
-In adition to removing the check from the individual panels, it is believed
-that even the core shouldn't need this check and that should be considered
-an error if other code ever relies on this check. The check in the core
-currently prints a warning whenever something is relying on this check with
-dev_warn(). After a little while, we likely want to promote this to a
-WARN(1) to help encourage folks not to rely on this behavior.
-
-Contact: Douglas Anderson
-
-Level: Starter/Intermediate
-
Core refactorings
=
--
2.25.1
[RFC] Clean up check for already prepared panel
Since the core function drm_panel_prepare already checks if the
panel is prepared, we can remove this duplicate check from tm5p5_nt35596_prepare
which acts as a no-op. As suggested in the GPU TODO [1]
[1]
https://docs.kernel.org/gpu/todo.html#clean-up-checks-for-already-prepared-enabled-in-panels
Suggested-by: Douglas Anderson
Signed-off-by: Yuran Pereira
---
drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
index 075a7af81eff..af83451b3374 100644
--- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
+++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
@@ -112,9 +112,6 @@ static int tm5p5_nt35596_prepare(struct drm_panel *panel)
struct device *dev = >dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
if (ret < 0) {
dev_err(dev, "Failed to enable regulators: %d\n", ret);
@@ -132,7 +129,6 @@ static int tm5p5_nt35596_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
--
2.25.1
