[PATCH] drm: mali-dp: Add check for kzalloc

[PATCH] drm: mali-dp: Add check for kzalloc

[Jiasheng Jiang]

As kzalloc may fail and return NULL pointer, it should be better to check
the return value in order to avoid the NULL pointer dereference in
__drm_atomic_helper_connector_reset.

Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index ef76d0e6ee2f..fe4474c2ddcf 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
 		__drm_atomic_helper_connector_destroy_state(connector->state);
 
 	kfree(connector->state);
-	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+
+	if (mw_state)
+		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+	else
+		__drm_atomic_helper_connector_reset(connector, NULL);
 }
 
 static enum drm_connector_status
-- 
2.25.1

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Liviu Dudau]

On Wed, Dec 07, 2022 at 05:21:18PM +0800, Jiasheng Jiang wrote:
> As kzalloc may fail and return NULL pointer, it should be better to check
> the return value in order to avoid the NULL pointer dereference in
> __drm_atomic_helper_connector_reset.
> 
> Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>

Thanks for catching this!


Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu


> ---
>  drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index ef76d0e6ee2f..fe4474c2ddcf 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
>  		__drm_atomic_helper_connector_destroy_state(connector->state);
>  
>  	kfree(connector->state);
> -	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +
> +	if (mw_state)
> +		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +	else
> +		__drm_atomic_helper_connector_reset(connector, NULL);
>  }
>  
>  static enum drm_connector_status
> -- 
> 2.25.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Robin Murphy]

On 2022-12-07 09:21, Jiasheng Jiang wrote:
> As kzalloc may fail and return NULL pointer, it should be better to check
> the return value in order to avoid the NULL pointer dereference in
> __drm_atomic_helper_connector_reset.

This commit message is nonsense; if 
__drm_atomic_helper_connector_reset() would dereference the NULL implied 
by &mw_state->base, it would equally still dereference the explicit NULL 
pointer passed after this patch.

The current code works out OK because "base" is the first member of 
struct malidp_mw_connector_state, thus if mw_state is NULL then 
&mw_state->base == NULL + 0 == NULL. Now you *could* argue that this 
isn't robust if the layout of struct malidp_mw_connector_state ever 
changes, and that could be a valid justification for making this change, 
but the reason given certainly isn't.

Arithmetic on a (potentially) NULL pointer may well be a sign that it's 
worth a closer look to check whether it really is what the code intended 
to do, but don't automatically assume it has to be a bug. Otherwise, 
good luck with "fixing" every user of container_of() throughout the 
entire kernel.

Thanks,
Robin.

> Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>   drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index ef76d0e6ee2f..fe4474c2ddcf 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
>   		__drm_atomic_helper_connector_destroy_state(connector->state);
>   
>   	kfree(connector->state);
> -	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +
> +	if (mw_state)
> +		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +	else
> +		__drm_atomic_helper_connector_reset(connector, NULL);
>   }
>   
>   static enum drm_connector_status

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Liviu Dudau]

On Wed, Dec 07, 2022 at 01:59:04PM +0000, Robin Murphy wrote:
> On 2022-12-07 09:21, Jiasheng Jiang wrote:
> > As kzalloc may fail and return NULL pointer, it should be better to check
> > the return value in order to avoid the NULL pointer dereference in
> > __drm_atomic_helper_connector_reset.
> 
> This commit message is nonsense; if __drm_atomic_helper_connector_reset()
> would dereference the NULL implied by &mw_state->base, it would equally
> still dereference the explicit NULL pointer passed after this patch.

Where?

> 
> The current code works out OK because "base" is the first member of struct
> malidp_mw_connector_state, thus if mw_state is NULL then &mw_state->base ==
> NULL + 0 == NULL. Now you *could* argue that this isn't robust if the layout
> of struct malidp_mw_connector_state ever changes, and that could be a valid
> justification for making this change, but the reason given certainly isn't.

I appreciate the input and I agree with your analysis, however I don't have the same
confidence that compilers will always do the NULL + 0 math to get address of base.
Would this always work when you have authenticated pointers or is the compiler going
to generate some plumbing code that checks the pointer before doing the math?

> 
> Arithmetic on a (potentially) NULL pointer may well be a sign that it's
> worth a closer look to check whether it really is what the code intended to
> do, but don't automatically assume it has to be a bug. Otherwise, good luck
> with "fixing" every user of container_of() throughout the entire kernel.

My understanding is that you're supposed to use container_of() only when you're sure
that your pointer is valid. container_of_safe() seems to be the one to use when you
don't care about NULL pointers.

Best regards,
Liviu

> 
> Thanks,
> Robin.
> 
> > Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
> > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> > ---
> >   drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> > index ef76d0e6ee2f..fe4474c2ddcf 100644
> > --- a/drivers/gpu/drm/arm/malidp_mw.c
> > +++ b/drivers/gpu/drm/arm/malidp_mw.c
> > @@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
> >   		__drm_atomic_helper_connector_destroy_state(connector->state);
> >   	kfree(connector->state);
> > -	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> > +
> > +	if (mw_state)
> > +		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> > +	else
> > +		__drm_atomic_helper_connector_reset(connector, NULL);
> >   }
> >   static enum drm_connector_status

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Robin Murphy]

On 2022-12-07 15:29, Liviu Dudau wrote:
> On Wed, Dec 07, 2022 at 01:59:04PM +0000, Robin Murphy wrote:
>> On 2022-12-07 09:21, Jiasheng Jiang wrote:
>>> As kzalloc may fail and return NULL pointer, it should be better to check
>>> the return value in order to avoid the NULL pointer dereference in
>>> __drm_atomic_helper_connector_reset.
>>
>> This commit message is nonsense; if __drm_atomic_helper_connector_reset()
>> would dereference the NULL implied by &mw_state->base, it would equally
>> still dereference the explicit NULL pointer passed after this patch.
> 
> Where?

Exactly, that function already checks conn_state for NULL anyway, so any 
reasoning based on it not doing that is clearly erroneous. Even if 
something else changed in future to actually make this a bug, it still 
wouldn't strictly dereference NULL, but some small non-NULL value.

>> The current code works out OK because "base" is the first member of struct
>> malidp_mw_connector_state, thus if mw_state is NULL then &mw_state->base ==
>> NULL + 0 == NULL. Now you *could* argue that this isn't robust if the layout
>> of struct malidp_mw_connector_state ever changes, and that could be a valid
>> justification for making this change, but the reason given certainly isn't.
> 
> I appreciate the input and I agree with your analysis, however I don't have the same
> confidence that compilers will always do the NULL + 0 math to get address of base.
> Would this always work when you have authenticated pointers or is the compiler going
> to generate some plumbing code that checks the pointer before doing the math?

For the current definition of struct malidp_mw_connector_state, 
&mw_state->base is equal to mw_state, that's just how C works:

"A pointer to a structure object, suitably converted, points to its 
initial member (or if that member is a bit-field, then to the unit in 
which it resides), and vice versa. There may be unnamed padding within a 
structure object, but not at its beginning."

Indeed a C compiler is technically at liberty to make checks for whether 
any pointer points to a valid object when evaluating it, but in practice 
no compiler is going to do that because it would be horrendously 
inefficient, and since the behaviour of dereferencing an invalid pointer 
is undefined, compilers are also able to simply assume all pointers are 
valid and generate good code based on that. Don't forget that there are 
several compiler optimisations that Linux actually depends on; AFAICT 
this is one of them.

>> Arithmetic on a (potentially) NULL pointer may well be a sign that it's
>> worth a closer look to check whether it really is what the code intended to
>> do, but don't automatically assume it has to be a bug. Otherwise, good luck
>> with "fixing" every user of container_of() throughout the entire kernel.
> 
> My understanding is that you're supposed to use container_of() only when you're sure
> that your pointer is valid. container_of_safe() seems to be the one to use when you
> don't care about NULL pointers.

I was thinking more along the lines of the "((type *)0)->member" 
expression in the definition, but fair enough, that's perhaps not the 
best example since you can argue it's an operand of typeof() which won't 
actually be evaluated. Try `git grep '&((.\+ *)\(0\|NULL\))->'` for more 
examples that will be. If none of those are going to work as intended, 
the kernel likely has bigger problems than how one driver might behave 
in OOM conditions.

Anyway, like I say I'm not objecting to the code change - even if the 
current non-bug wasn't an oversight, it's still a bit too clever for its 
own good. However, if the *justification* for making that change is 
going to go beyond "do this because static analysis suggested it", then 
it needs to explain a potential issue that actually exists and is worthy 
of fixing, not make up one that doesn't.

Cheers,
Robin.

[PATCH] drm: mali-dp: Add check for kzalloc

[Jiasheng Jiang]

As kzalloc may fail and return NULL pointer, the "mw_state" can be NULL.
If the the layout of struct malidp_mw_connector_state ever changes, it
will cause NULL poineter derefernce of "&mw_state->base".
Therefore, the "mw_state" should be check whether it is NULL in order
to improve the robust.

Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index ef76d0e6ee2f..c74c7c4e6006 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
 		__drm_atomic_helper_connector_destroy_state(connector->state);
 
 	kfree(connector->state);
-	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+
+	if (mw_state)
+		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+	else:
+		__drm_atomic_helper_connector_reset(connector, NULL);
 }
 
 static enum drm_connector_status
-- 
2.25.1

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Jiasheng Jiang]

On Wed, Dec 07, 2022 at 09:59:04PM +0800, Robin Murphy wrote:
>> As kzalloc may fail and return NULL pointer, it should be better to check
>> the return value in order to avoid the NULL pointer dereference in
>> __drm_atomic_helper_connector_reset.
> 
> This commit message is nonsense; if 
> __drm_atomic_helper_connector_reset() would dereference the NULL implied 
> by &mw_state->base, it would equally still dereference the explicit NULL 
> pointer passed after this patch.
> 
> The current code works out OK because "base" is the first member of 
> struct malidp_mw_connector_state, thus if mw_state is NULL then 
> &mw_state->base == NULL + 0 == NULL. Now you *could* argue that this 
> isn't robust if the layout of struct malidp_mw_connector_state ever 
> changes, and that could be a valid justification for making this change, 
> but the reason given certainly isn't.
> 
> Arithmetic on a (potentially) NULL pointer may well be a sign that it's 
> worth a closer look to check whether it really is what the code intended 
> to do, but don't automatically assume it has to be a bug. Otherwise, 
> good luck with "fixing" every user of container_of() throughout the 
> entire kernel.

I have sent a new patch with the modified commit mesage.

Thanks,
Jiang

[PATCH] drm: mali-dp: Add check for kzalloc

[Jiasheng Jiang]

As kzalloc may fail and return NULL pointer, the "mw_state" can be NULL.
If the layout of struct malidp_mw_connector_state ever changes, it
will cause NULL poineter derefernce of "&mw_state->base".
Therefore, the "mw_state" should be check whether it is NULL in order
to improve the robust.

Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index ef76d0e6ee2f..fe4474c2ddcf 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
 		__drm_atomic_helper_connector_destroy_state(connector->state);
 
 	kfree(connector->state);
-	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+
+	if (mw_state)
+		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+	else
+		__drm_atomic_helper_connector_reset(connector, NULL);
 }
 
 static enum drm_connector_status
-- 
2.25.1

[PATCH] drm: mali-dp: Add check for kzalloc

[Jiasheng Jiang]

As kzalloc may fail and return NULL pointer, the "mw_state" can be NULL.
If the layout of struct malidp_mw_connector_state ever changes, it
will cause NULL poineter derefernce of "&mw_state->base".
Therefore, the "mw_state" should be checked whether it is NULL in order
to improve the robust.

Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
index ef76d0e6ee2f..fe4474c2ddcf 100644
--- a/drivers/gpu/drm/arm/malidp_mw.c
+++ b/drivers/gpu/drm/arm/malidp_mw.c
@@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
 		__drm_atomic_helper_connector_destroy_state(connector->state);
 
 	kfree(connector->state);
-	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+
+	if (mw_state)
+		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
+	else
+		__drm_atomic_helper_connector_reset(connector, NULL);
 }
 
 static enum drm_connector_status
-- 
2.25.1

Re: [PATCH] drm: mali-dp: Add check for kzalloc

[Liviu Dudau]

Hi Jiasheng,

I appreciate the effort you have put into this and I find nothing wrong with the
intention of the patch. However, I don't intend to move base from being the first
member of the malidp_mw_connector_state struct as it has other benefits in the code
and we can use container_of() in implementation of generic APIs. As Robin has rightly
pointed, it is unlikely that a compiler will dereference the pointer before doing
offset_of(), so having mw_state == NULL is safe, even if quirky.

So I'm going to thank you for the patch but I will not merge it.

As a side comment, please use --in-reply-to and link to previous email when re-sending
patches with small spelling fixes as otherwise it gets confusing on which email is the last
one and it also relies on the servers delivering messages in the order you've sent,
not always a strong guarantee.

Best regards,
Liviu

On Thu, Dec 08, 2022 at 11:16:21AM +0800, Jiasheng Jiang wrote:
> As kzalloc may fail and return NULL pointer, the "mw_state" can be NULL.
> If the layout of struct malidp_mw_connector_state ever changes, it
> will cause NULL poineter derefernce of "&mw_state->base".
> Therefore, the "mw_state" should be checked whether it is NULL in order
> to improve the robust.
> 
> Fixes: 8cbc5caf36ef ("drm: mali-dp: Add writeback connector")
> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  drivers/gpu/drm/arm/malidp_mw.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index ef76d0e6ee2f..fe4474c2ddcf 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -72,7 +72,11 @@ static void malidp_mw_connector_reset(struct drm_connector *connector)
>  		__drm_atomic_helper_connector_destroy_state(connector->state);
>  
>  	kfree(connector->state);
> -	__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +
> +	if (mw_state)
> +		__drm_atomic_helper_connector_reset(connector, &mw_state->base);
> +	else
> +		__drm_atomic_helper_connector_reset(connector, NULL);
>  }
>  
>  static enum drm_connector_status
> -- 
> 2.25.1
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯