Bug 189111 - Function add_grefs() may return improper value on failures
Summary: Function add_grefs() may return improper value on failures
Status: RESOLVED CODE_FIX
Alias: None
Product: Virtualization
Classification: Unclassified
Component: Xen (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: virtualization_xen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-25 11:24 UTC by bianpan
Modified: 2017-05-12 00:31 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.62 KB, patch)
2017-05-12 00:31 UTC, bianpan
Details | Diff

Description bianpan 2016-11-25 11:24:28 UTC
In function add_grefs() defined in file drivers/xen/gntalloc.c, kzalloc() is called at line 132, and the return value is checked against NULL. If the return value is NULL, the control flow jumps to label "undo", and returns variable rc at the end. Variable rc is initialized with "-ENOMEM" at line 130, but it is reset in the loop (at line 144), and after the check at line 147, its value must be larger than or equal to 0. As a result, the return value will be larger than or equal to 0 if kzalloc() returns NULL during the second or after repeat of the loop body. In this case, its caller, i.e. gntalloc_ioctl_alloc(), cannot detect the error. This may result in unexpected bugs. For example, when add_grefs() fails, the rest elements of parameter @@gref_ids keep uninitialized, however, if function gntalloc_ioctl_alloc() does not detect the error, elements of gref_ids will be copied to user space (see line 331), resulting in kernel information leaks.
There is another similar bug when the call to alloc_page() (at line 139) fails. Codes related to these two bugs are summarised as follows.

add_grefs @@ drivers/xen/gntalloc.c
121 static int add_grefs(struct ioctl_gntalloc_alloc_gref *op,
122     uint32_t *gref_ids, struct gntalloc_file_private_data *priv)
123 {
        ...
130     rc = -ENOMEM;
131     for (i = 0; i < op->count; i++) {
            // Insert "rc = -ENOMEM;" here?
132         gref = kzalloc(sizeof(*gref), GFP_KERNEL);
133         if (!gref)
                //Bug (1): the value of rc may not be negative
134             goto undo;
135         list_add_tail(&gref->next_gref, &queue_gref);
136         list_add_tail(&gref->next_file, &queue_file);
137         gref->users = 1;
138         gref->file_index = op->index + i * PAGE_SIZE;
139         gref->page = alloc_page(GFP_KERNEL|__GFP_ZERO);
140         if (!gref->page)
                //Bug (2): the value of rc may not be negative
141             goto undo;
142 
143         /* Grant foreign access to the page. */
144         rc = gnttab_grant_foreign_access(op->domid,
145                          xen_page_to_gfn(gref->page),
146                          readonly);
147         if (rc < 0)
148             goto undo;
149         gref_ids[i] = gref->gref_id = rc;
150     }
        ...
158     return 0;
159
160 undo:
        ...
178     return rc;
179 }


gntalloc_ioctl_alloc @@ drivers/xen/gntalloc.c
280 static long gntalloc_ioctl_alloc(struct gntalloc_file_private_data *priv,
281         struct ioctl_gntalloc_alloc_gref __user *arg)
282 {
283     int rc = 0;
        ...
        // gref_ids: the memory is not initialized 
294     gref_ids = kcalloc(op.count, sizeof(gref_ids[0]), GFP_TEMPORARY);
295     if (!gref_ids) {
296         rc = -ENOMEM;
297         goto out;
298     }
        ...
316     rc = add_grefs(&op, gref_ids, priv);
317     if (rc < 0)
318         goto out_free;
319 
320     /* Once we finish add_grefs, it is unsafe to touch the new reference,
321      * since it is possible for a concurrent ioctl to remove it (by guessing
322      * its index). If the userspace application doesn't provide valid memory
323      * to write the IDs to, then it will need to close the file in order to
324      * release - which it will do by segfaulting when it tries to access the
325      * IDs to close them.
326      */
327     if (copy_to_user(arg, &op, sizeof(op))) {
328         rc = -EFAULT;
329         goto out_free;
330     }
        // Bug: may copy uninitialized memory to user space
331     if (copy_to_user(arg->gref_ids, gref_ids,
332             sizeof(gref_ids[0]) * op.count)) {
333         rc = -EFAULT;
334         goto out_free;
335     }
336 
337 out_free:
338     kfree(gref_ids);
339 out:
340     return rc;
341 }

Thanks very much!
Comment 1 bianpan 2017-05-12 00:31:43 UTC
Created attachment 256459 [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.