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!
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.