| Message ID | 20240823163513.4344-1-hanchunchao@inspur.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: himax-hx83102: Add NULL pointer check in hx83102_get_modes | expand |
Hi, On Fri, Aug 23, 2024 at 9:35 AM Charles Han <hanchunchao@inspur.com> wrote: > > In hx83102_get_modes(), the return value of drm_mode_duplicate() > is assigned to mode, which will lead to a possible NULL pointer > dereference on failure of drm_mode_duplicate(). Even though a > small allocation failing is basically impossible, kernel policy > is still to check for NULL so add the check. > > Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate driver") > Signed-off-by: Charles Han <hanchunchao@inspur.com> > --- > drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 ++ > 1 file changed, 2 insertions(+) FWIW, this looks to be v4 of your patch, right? The subject line should include a version number and you should be providing version history "after the cut" in your patch. Tools like "b4" and "patman" can help you get this correct [1]. If you plan to continue posting patches you'll need to start getting this right. The next version of your patch would be v5. [1] https://sched.co/1aBGS I see: v1: https://lore.kernel.org/r/20240821095039.15282-1-hanchunchao@inspur.com v2: https://lore.kernel.org/r/20240822093442.4262-1-hanchunchao@inspur.com v3: https://lore.kernel.org/r/20240823083657.7100-1-hanchunchao@inspur.com > diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c > index 6e4b7e4644ce..e67555323d3b 100644 > --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c > +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c > @@ -565,6 +565,8 @@ static int hx83102_get_modes(struct drm_panel *panel, > struct drm_display_mode *mode; > > mode = drm_mode_duplicate(connector->dev, m); > + if (!mode) > + return -EINVAL; I would have returned -ENOMEM since drm_mode_duplicate() is defined to allocate memory copy the mode (like strdup does for strings) and it should be clear that the only failure case is failure to allocate memory. Other callers convert a NULL return as -ENOMEM. -Doug
Hi, On Fri, Aug 23, 2024 at 9:49 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Aug 23, 2024 at 9:35 AM Charles Han <hanchunchao@inspur.com> wrote: > > > > In hx83102_get_modes(), the return value of drm_mode_duplicate() > > is assigned to mode, which will lead to a possible NULL pointer > > dereference on failure of drm_mode_duplicate(). Even though a > > small allocation failing is basically impossible, kernel policy > > is still to check for NULL so add the check. > > > > Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate driver") > > Signed-off-by: Charles Han <hanchunchao@inspur.com> > > --- > > drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 ++ > > 1 file changed, 2 insertions(+) > > FWIW, this looks to be v4 of your patch, right? The subject line > should include a version number and you should be providing version > history "after the cut" in your patch. Tools like "b4" and "patman" > can help you get this correct [1]. If you plan to continue posting > patches you'll need to start getting this right. The next version of > your patch would be v5. > > [1] https://sched.co/1aBGS > > I see: > > v1: https://lore.kernel.org/r/20240821095039.15282-1-hanchunchao@inspur.com > v2: https://lore.kernel.org/r/20240822093442.4262-1-hanchunchao@inspur.com > v3: https://lore.kernel.org/r/20240823083657.7100-1-hanchunchao@inspur.com > > > > diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c > > index 6e4b7e4644ce..e67555323d3b 100644 > > --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c > > +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c > > @@ -565,6 +565,8 @@ static int hx83102_get_modes(struct drm_panel *panel, > > struct drm_display_mode *mode; > > > > mode = drm_mode_duplicate(connector->dev, m); > > + if (!mode) > > + return -EINVAL; > > I would have returned -ENOMEM since drm_mode_duplicate() is defined to > allocate memory copy the mode (like strdup does for strings) and it > should be clear that the only failure case is failure to allocate > memory. Other callers convert a NULL return as -ENOMEM. FWIW: if you spin v5 of this patch and have it return -ENOMEM then I'm happy to apply it. -Doug
diff --git a/drivers/gpu/drm/panel/panel-himax-hx83102.c b/drivers/gpu/drm/panel/panel-himax-hx83102.c index 6e4b7e4644ce..e67555323d3b 100644 --- a/drivers/gpu/drm/panel/panel-himax-hx83102.c +++ b/drivers/gpu/drm/panel/panel-himax-hx83102.c @@ -565,6 +565,8 @@ static int hx83102_get_modes(struct drm_panel *panel, struct drm_display_mode *mode; mode = drm_mode_duplicate(connector->dev, m); + if (!mode) + return -EINVAL; mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_set_name(mode);
In hx83102_get_modes(), the return value of drm_mode_duplicate() is assigned to mode, which will lead to a possible NULL pointer dereference on failure of drm_mode_duplicate(). Even though a small allocation failing is basically impossible, kernel policy is still to check for NULL so add the check. Fixes: 0ef94554dc40 ("drm/panel: himax-hx83102: Break out as separate driver") Signed-off-by: Charles Han <hanchunchao@inspur.com> --- drivers/gpu/drm/panel/panel-himax-hx83102.c | 2 ++ 1 file changed, 2 insertions(+)