Bug 188911 - Function qxl_release_alloc() returns an improper value when the call to kmalloc() fails, resulting in bad memory access
Summary: Function qxl_release_alloc() returns an improper value when the call to kmall...
Status: RESOLVED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Video(DRI - non Intel) (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: drivers_video-dri
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-25 11:08 UTC by bianpan
Modified: 2017-05-12 00:09 UTC (History)
0 users

See Also:
Kernel Version: linux-4.9-rc6
Subsystem:
Regression: No
Bisected commit-id:


Attachments
The patch fixes the bug (1.20 KB, patch)
2017-05-12 00:09 UTC, bianpan
Details | Diff

Description bianpan 2016-11-25 11:08:20 UTC
(1) Function kmalloc() return a NULL pointer if there is no enough memory. The function qxl_release_alloc() defined in file drivers/gpu/drm/qxl/qxl_release.c tries to allocate memory and stores in its third parameter @@ret. Parameter @@ret keeps unmodified if the call to kmalloc() (at line 133) fails. In this case, it returns 0.
(2) Function qxl_alloc_release_reserved() calls qxl_release_alloc() to allocate memory for its parameter @@release. By reviewing the source code of the callers of function qxl_alloc_release_reserved() (e.g. qxl_process_single_command() defined in file drivers/gpu/drm/qxl/qxl_ioctl.c), we find that parameter @@release is uninitialized. The return value of qxl_release_alloc() is checked, if the return value is 0, the execution flow will continue, and the memory pointed by @@release will be accessed (at line 368). Recall that function qxl_release_alloc() returns 0 when kmalloc() fails. In this case, the uninitialized memory will be accessed, causing bad memory access.
(3) To avoid bad memory access, it's better to return "-ENOMEM" when the call to kmalloc() fails in function qxl_release_alloc().

Codes related to this bug are summarised as follows.
(1) qxl_release_alloc @@ drivers/gpu/drm/qxl/qxl_release.c
125 static int
126 qxl_release_alloc(struct qxl_device *qdev, int type,
127           struct qxl_release **ret)
128 {
129     struct qxl_release *release;
130     int handle;
131     size_t size = sizeof(*release);
132 
133     release = kmalloc(size, GFP_KERNEL);
134     if (!release) {
135         DRM_ERROR("Out of memory\n");
136         return 0;  // "return -ENOMEM;"?
137     }
        ...
155     *ret = release;
156     QXL_INFO(qdev, "allocated release %d\n", handle);
157     release->id = handle;
158     return handle;
159 }

(2) qxl_alloc_release_reserved @@ drivers/gpu/drm/qxl/qxl_release.c
323 int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
324                        int type, struct qxl_release **release,
325                        struct qxl_bo **rbo)
326 {
327     struct qxl_bo *bo;
328     int idr_ret;
        ...
344     idr_ret = qxl_release_alloc(qdev, type, release);
345     if (idr_ret < 0) {
346         if (rbo)
347             *rbo = NULL;
348         return idr_ret;
349     }
        ...
366     bo = qxl_bo_ref(qdev->current_release_bo[cur_idx]);
367 
        // bad memory access when kmalloc() fails?
368     (*release)->release_offset = qdev->current_release_bo_offset[cur_idx] * release_size_per_bo[cur_idx];
369     qdev->current_release_bo_offset[cur_idx]++;
        ...
388 }

(3) qxl_process_single_command @@ drivers/gpu/drm/qxl/qxl_ioctl.c
138 static int qxl_process_single_command(struct qxl_device *qdev,
139                                       struct drm_qxl_command *cmd,
140                                       struct drm_file *file_priv)
141 {
142         struct qxl_reloc_info *reloc_info;
143         int release_type;
144         struct qxl_release *release; // release is not initialized
            ...
175         ret = qxl_alloc_release_reserved(qdev,
176                                          sizeof(union qxl_release_info) +
177                                          cmd->command_size,
178                                          release_type,
179                                          &release,
180                                          &cmd_bo);
181         if (ret)
182                 goto out_free_reloc;
            ...
269 out_free_reloc:
270         kfree(reloc_info);
271         return ret;
272 }

Thanks very much!
Comment 1 bianpan 2017-05-12 00:09:18 UTC
Created attachment 256435 [details]
The patch fixes the bug

The patch has been merged into the latest version of the Linux kernel. So I will close the bug.

Note You need to log in before you can comment on or make changes to this bug.