[PATCH v1 1/1] drm: Multiple Null pointer dereference [null-pointer-deref] (CWE 476) problems:
Joe Moriarty
joe.moriarty at oracle.com
Fri Feb 9 14:46:32 UTC 2018
On 2/9/2018 7:17 AM, Jani Nikula wrote:
> On Thu, 08 Feb 2018, Joe Moriarty <joe.moriarty at oracle.com> wrote:
>> The Parfait (version 2.1.0) static code analysis tool found multiple NULL
>> pointer derefernce problems.
>
> Thanks for the patch. Multiple problems requires multiple patches to fix
> them, one patch per problem. Please split up the patch.
>
> Thanks,
> Jani.
>
>>
>> - drivers/gpu/drm/drm_dp_mst_topology.c
>> The call to drm_dp_calculate_rad() in function drm_dp_port_setup_pdt()
>> could result in a NULL pointer being returned to port->mstb due to a
>> failure to allocate memory for port->mstb.
>>
>> - drivers/gpu/drm/drm_drv.c
>> Any calls to drm_minor_get_slot() could result in the return of a NULL
>> pointer when an invalid DRM device type is encountered. 2 helper
>> functions where added for pointer manipulation (drm_minor_get_slot()
>> and drm_minor_set_minor()) along with checks for valid pointers for
>> struct drm_device variables throughout this module.
>>
>> - drivers/gpu/drm/drm_edid.c
>> The call to drm_cvt_mode() in function drm_mode_std() for the
>> HDTV hack resulted in the possibility of accessing a NULL pointer
>> if drm_mode_std() returned NULL. A check for this added right after
>> the call to drm_cvt_mode() in this particular area of code.
>>
>> - drivers/gpu/drm/drm_vblank.c
>> Null pointer checks were added to return values from calls to
>> drm_crtc_from_index(). There is a possibility, however minute, that
>> crtc->index may not be found when trying to find the struct crtc
>> from it's assigned index given in drm_crtc_init_with_planes().
>> 3 return checks for NULL where added.
>>
>> Signed-off-by: Joe Moriarty <joe.moriarty at oracle.com>
>> Reviewed-by: Steven Sistare <steven.sistare at oracle.com>
>> ---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 8 +++++---
>> drivers/gpu/drm/drm_drv.c | 38 +++++++++++++++++++++++++++++++----
>> drivers/gpu/drm/drm_edid.c | 2 ++
>> drivers/gpu/drm/drm_vblank.c | 6 +++---
>> 4 files changed, 44 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 70dcfa58d3c2..ec503d416062 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -1082,10 +1082,12 @@ static bool drm_dp_port_setup_pdt(struct drm_dp_mst_port *port)
>> lct = drm_dp_calculate_rad(port, rad);
>>
>> port->mstb = drm_dp_add_mst_branch_device(lct, rad);
>> - port->mstb->mgr = port->mgr;
>> - port->mstb->port_parent = port;
>> + if (port->mstb) {
>> + port->mstb->mgr = port->mgr;
>> + port->mstb->port_parent = port;
>>
>> - send_link = true;
>> + send_link = true;
>> + }
>> break;
>> }
>> return send_link;
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 9acc1e157813..938eee77f014 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -99,10 +99,36 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
>> case DRM_MINOR_CONTROL:
>> return &dev->control;
>> default:
>> + DRM_ERROR("Error in %s: Invalid dev, type = %d\n",
>> + __func__, type);
>> return NULL;
>> }
>> }
>>
>> +static inline int drm_minor_set_minor(struct drm_device *dev,
>> + unsigned int type,
>> + struct drm_minor *minor)
>> +{
>> + struct drm_minor **slot = drm_minor_get_slot(dev, type);
>> + int retval = -ENODEV;
>> +
>> + if (slot) {
>> + retval = 0;
>> + *slot = minor;
>> + }
>> + return retval;
>> +}
>> +
>> +static inline struct drm_minor *drm_minor_get_minor(struct drm_device *dev,
>> + unsigned int type)
>> +{
>> + struct drm_minor **slot = drm_minor_get_slot(dev, type);
>> +
>> + if (slot)
>> + return *slot;
>> + return NULL;
>> +}
>> +
>> static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>> {
>> struct drm_minor *minor;
>> @@ -137,8 +163,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>> goto err_index;
>> }
>>
>> - *drm_minor_get_slot(dev, type) = minor;
>> - return 0;
>> + r = drm_minor_set_minor(dev, type, minor);
>> + if (r == 0)
>> + return r;
>>
>> err_index:
>> spin_lock_irqsave(&drm_minor_lock, flags);
>> @@ -155,6 +182,9 @@ static void drm_minor_free(struct drm_device *dev, unsigned int type)
>> unsigned long flags;
>>
>> slot = drm_minor_get_slot(dev, type);
>> + if (!slot)
>> + return
>> +
>> minor = *slot;
>> if (!minor)
>> return;
>> @@ -177,7 +207,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
>>
>> DRM_DEBUG("\n");
>>
>> - minor = *drm_minor_get_slot(dev, type);
>> + minor = drm_minor_get_slot(dev, type);
>> if (!minor)
>> return 0;
>>
>> @@ -209,7 +239,7 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
>> struct drm_minor *minor;
>> unsigned long flags;
>>
>> - minor = *drm_minor_get_slot(dev, type);
>> + minor = drm_minor_get_slot(dev, type);
>> if (!minor || !device_is_registered(minor->kdev))
>> return;
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index ddd537914575..23c9977d8999 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2083,6 +2083,8 @@ drm_mode_std(struct drm_connector *connector, struct edid *edid,
>> if (hsize == 1366 && vsize == 768 && vrefresh_rate == 60) {
>> mode = drm_cvt_mode(dev, 1366, 768, vrefresh_rate, 0, 0,
>> false);
>> + if (!mode)
>> + return NULL;
>> mode->hdisplay = 1366;
>> mode->hsync_start = mode->hsync_start - 1;
>> mode->hsync_end = mode->hsync_end - 1;
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 32d9bcf5be7f..a3a1bce87468 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -120,7 +120,7 @@ static u32 __get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>>
>> - if (crtc->funcs->get_vblank_counter)
>> + if (crtc && crtc->funcs->get_vblank_counter)
>> return crtc->funcs->get_vblank_counter(crtc);
>> }
>>
>> @@ -318,7 +318,7 @@ static void __disable_vblank(struct drm_device *dev, unsigned int pipe)
>> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>>
>> - if (crtc->funcs->disable_vblank) {
>> + if (crtc && crtc->funcs->disable_vblank) {
>> crtc->funcs->disable_vblank(crtc);
>> return;
>> }
>> @@ -918,7 +918,7 @@ static int __enable_vblank(struct drm_device *dev, unsigned int pipe)
>> if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe);
>>
>> - if (crtc->funcs->enable_vblank)
>> + if (crtc && crtc->funcs->enable_vblank)
>> return crtc->funcs->enable_vblank(crtc);
>> }
>
Thanks for the time to review Jani. I will split the patch up on the
next version.
Thanks,
Joe
More information about the dri-devel
mailing list