[PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[Ma Ke]

In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is
assigned to modeset->mode, which will lead to a possible NULL pointer
dereference on failure of drm_mode_duplicate(). Add a check to avoid npd.

Cc: stable@vger.kernel.org
Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
Changes in v3:
- modified patch as suggestions, returned error directly when failing to 
get modeset->mode.
Changes in v2:
- added the recipient's email address, due to the prolonged absence of a 
response from the recipients.
- added Cc stable.
---
 drivers/gpu/drm/drm_client_modeset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
index 31af5cf37a09..750b8dce0f90 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -880,6 +880,9 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
 
 			kfree(modeset->mode);
 			modeset->mode = drm_mode_duplicate(dev, mode);
+			if (!modeset->mode)
+				return 0;
+
 			drm_connector_get(connector);
 			modeset->connectors[modeset->num_connectors++] = connector;
 			modeset->x = offset->x;
-- 
2.25.1

Re: [PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[Jani Nikula]

On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
> In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is
> assigned to modeset->mode, which will lead to a possible NULL pointer
> dereference on failure of drm_mode_duplicate(). Add a check to avoid npd.
>
> Cc: stable@vger.kernel.org
> Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
> Changes in v3:
> - modified patch as suggestions, returned error directly when failing to 
> get modeset->mode.

This is not what I suggested, and you can't just return here either.

BR,
Jani.


> Changes in v2:
> - added the recipient's email address, due to the prolonged absence of a 
> response from the recipients.
> - added Cc stable.
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 31af5cf37a09..750b8dce0f90 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -880,6 +880,9 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
>  
>  			kfree(modeset->mode);
>  			modeset->mode = drm_mode_duplicate(dev, mode);
> +			if (!modeset->mode)
> +				return 0;
> +
>  			drm_connector_get(connector);
>  			modeset->connectors[modeset->num_connectors++] = connector;
>  			modeset->x = offset->x;

-- 
Jani Nikula, Intel

Re: [PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[Ma Ke]

On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
> > In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is
> > assigned to modeset->mode, which will lead to a possible NULL pointer
> > dereference on failure of drm_mode_duplicate(). Add a check to avoid npd.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> > ---
> > Changes in v3:
> > - modified patch as suggestions, returned error directly when failing to 
> > get modeset->mode.
> 
> This is not what I suggested, and you can't just return here either.
> 
> BR,
> Jani.
> 

I have carefully read through your comments. Based on your comments on the 
patchs I submitted, I am uncertain about the appropriate course of action 
following the return value check(whether to continue or to return directly,
as both are common approaches in dealing with function drm_mode_duplicate()
in Linux kernel, and such handling has received 'acked-by' in similar 
vulnerabilities). Could you provide some advice on this matter? Certainly, 
adding a return value check is essential, the reasons for which have been 
detailed in the vulnerability description. I am looking forward to your 
guidance and response. Thank you!

Best regards,

Ma Ke

> 
> > Changes in v2:
> > - added the recipient's email address, due to the prolonged absence of a 
> > response from the recipients.
> > - added Cc stable.
> > ---
> >  drivers/gpu/drm/drm_client_modeset.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> > index 31af5cf37a09..750b8dce0f90 100644
> > --- a/drivers/gpu/drm/drm_client_modeset.c
> > +++ b/drivers/gpu/drm/drm_client_modeset.c
> > @@ -880,6 +880,9 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
> >  
> >  			kfree(modeset->mode);
> >  			modeset->mode = drm_mode_duplicate(dev, mode);
> > +			if (!modeset->mode)
> > +				return 0;
> > +
> >  			drm_connector_get(connector);
> >  			modeset->connectors[modeset->num_connectors++] = connector;
> >  			modeset->x = offset->x;
> 
> -- 
> Jani Nikula, Intel

Re: [PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[Jani Nikula]

On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
> On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
>> > In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is
>> > assigned to modeset->mode, which will lead to a possible NULL pointer
>> > dereference on failure of drm_mode_duplicate(). Add a check to avoid npd.
>> >
>> > Cc: stable@vger.kernel.org
>> > Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
>> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
>> > ---
>> > Changes in v3:
>> > - modified patch as suggestions, returned error directly when failing to 
>> > get modeset->mode.
>> 
>> This is not what I suggested, and you can't just return here either.
>> 
>> BR,
>> Jani.
>> 
>
> I have carefully read through your comments. Based on your comments on the 
> patchs I submitted, I am uncertain about the appropriate course of action 
> following the return value check(whether to continue or to return directly,
> as both are common approaches in dealing with function drm_mode_duplicate()
> in Linux kernel, and such handling has received 'acked-by' in similar 
> vulnerabilities). Could you provide some advice on this matter? Certainly, 
> adding a return value check is essential, the reasons for which have been 
> detailed in the vulnerability description. I am looking forward to your 
> guidance and response. Thank you!

Everything depends on the context. You can't just go ahead and do the
same thing everywhere. If you handle errors, even the highly unlikely
ones such as this one, you better do it properly.

If you continue here, you'll still leave modeset->mode NULL. And you
don't propagate the error. Something else is going to hit the issue
soon.

If you return directly, you'll leave holding a few locks, and leaking
memory.

There's already some error handling in the function, in the same loop
even. Set ret = -ENOMEM and break.

(However, you could still argue there's an existing problem in the error
handling in that all modeset->connectors aren't put and cleaned up.)


BR,
Jani.







>
> Best regards,
>
> Ma Ke
>
>> 
>> > Changes in v2:
>> > - added the recipient's email address, due to the prolonged absence of a 
>> > response from the recipients.
>> > - added Cc stable.
>> > ---
>> >  drivers/gpu/drm/drm_client_modeset.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
>> > index 31af5cf37a09..750b8dce0f90 100644
>> > --- a/drivers/gpu/drm/drm_client_modeset.c
>> > +++ b/drivers/gpu/drm/drm_client_modeset.c
>> > @@ -880,6 +880,9 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
>> >  
>> >  			kfree(modeset->mode);
>> >  			modeset->mode = drm_mode_duplicate(dev, mode);
>> > +			if (!modeset->mode)
>> > +				return 0;
>> > +
>> >  			drm_connector_get(connector);
>> >  			modeset->connectors[modeset->num_connectors++] = connector;
>> >  			modeset->x = offset->x;
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel

Re: [PATCH v3] drm/client: fix null pointer dereference in drm_client_modeset_probe

[Ma Ke]

On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
> > On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >> On Wed, 24 Jul 2024, Ma Ke <make24@iscas.ac.cn> wrote:
> >> > In drm_client_modeset_probe(), the return value of drm_mode_duplicate() is
> >> > assigned to modeset->mode, which will lead to a possible NULL pointer
> >> > dereference on failure of drm_mode_duplicate(). Add a check to avoid npd.
> >> >
> >> > Cc: stable@vger.kernel.org
> >> > Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
> >> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> >> > ---
> >> > Changes in v3:
> >> > - modified patch as suggestions, returned error directly when failing to 
> >> > get modeset->mode.
> >> 
> >> This is not what I suggested, and you can't just return here either.
> >> 
> >> BR,
> >> Jani.
> >> 
> >
> > I have carefully read through your comments. Based on your comments on the 
> > patchs I submitted, I am uncertain about the appropriate course of action 
> > following the return value check(whether to continue or to return directly,
> > as both are common approaches in dealing with function drm_mode_duplicate()
> > in Linux kernel, and such handling has received 'acked-by' in similar 
> > vulnerabilities). Could you provide some advice on this matter? Certainly, 
> > adding a return value check is essential, the reasons for which have been 
> > detailed in the vulnerability description. I am looking forward to your 
> > guidance and response. Thank you!
> 
> Everything depends on the context. You can't just go ahead and do the
> same thing everywhere. If you handle errors, even the highly unlikely
> ones such as this one, you better do it properly.
> 
> If you continue here, you'll still leave modeset->mode NULL. And you
> don't propagate the error. Something else is going to hit the issue
> soon.
> 
> If you return directly, you'll leave holding a few locks, and leaking
> memory.
> 
> There's already some error handling in the function, in the same loop
> even. Set ret = -ENOMEM and break.
> 
> (However, you could still argue there's an existing problem in the error
> handling in that all modeset->connectors aren't put and cleaned up.)
> 
> 
> BR,
> Jani.

Indeed, it was my negligence. Thank you very much for your guidance. I will
carefully analyze according to your instructions and resubmit a new patch.

Best regards,

Ma Ke