Hey Zhou, Thanks for submitting this patch. On Tue, 30 Nov 2021 at 14:11, Zhou Qingyang <zhou1615@xxxxxxx> wrote: > > In cdns_mhdp_atomic_enable(), the return value of drm_mode_duplicate() > is assigned to mhdp_state->current_mode and used in drm_mode_set_name(). > There is a dereference of it in drm_mode_set_name(), which could lead > to a NULL pointer dereference on failure of drm_mode_duplicate(). > > Fix this bug by adding a check of mhdp_state->current_mode. > > This bug was found by a static analyzer. The analysis employs > differential checking to identify inconsistent security operations > (e.g., checks or kfrees) between two code paths and confirms that the > inconsistent operations are not recovered in the current function or > the callers, so they constitute bugs. > > Note that, as a bug found by static analysis, it can be a false > positive or hard to trigger. Multiple researchers have cross-reviewed > the bug. > > Builds with CONFIG_DRM_CDNS_MHDP8546=m show no new warnings, > and our static analyzer no longer warns about this code. > > Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP bridge") > Signed-off-by: Zhou Qingyang <zhou1615@xxxxxxx> > --- > drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > index 5530fbf64f1e..347fbecf76a4 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c > @@ -2040,6 +2040,11 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge, > mhdp_state = to_cdns_mhdp_bridge_state(new_state); > > mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode); > + if (!mhdp_state->current_mode) { > + ret = -ENOMEM; > + goto out; > + } > + This appears to be a problem that is handled in other drivers, but the solution here does strike me as good. The out-label will schedule modeset_retry_work to be executed if ret==-ENOMEM. If drm_mode_duplicate() fails, we've had a memory allocation issue, and failing is probably the correct solution here. However cdns_mhdp_atomic_enable() does allow for signalling failures. > drm_mode_set_name(mhdp_state->current_mode); > > dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__, mode->name); > -- > 2.25.1 >