On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> On Wed, 24 Jul 2024, Ma Ke <make24@xxxxxxxxxxx> wrote:
> > On Wed, 24 Jul 2024, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote:
> >> On Wed, 24 Jul 2024, Ma Ke <make24@xxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> >> > Fixes: cf13909aee05 ("drm/fb-helper: Move out modeset config code")
> >> > Signed-off-by: Ma Ke <make24@xxxxxxxxxxx>
> >> > ---
> >> > 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
