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

[Ma Ke <make24@iscas.ac.cn>]

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