On 4/12/19 3:47 PM, Sumit Gupta wrote: > > On 4/11/19 6:01 PM, Sumit Gupta wrote: >> From: sumitg <sumitg@xxxxxxxxxx> >> >> Fixing use-after-free within __v4l2_ctrl_handler_setup(). >> Memory is being freed with kfree(new_ref) for duplicate control >> reference entry but cluster is still referring to the duplicate entry. >> Change done to update ctrl->cluster only when new control reference is >> added. Also, checking if >> ctrl->ncontrols is non-zero before accessing controls from >> cluster pointer to avoid illegal access when cluster has zero >> controls. >> >> ================================================================== >> BUG: KASAN: use-after-free in __v4l2_ctrl_handler_setup+0x388/0x428 >> Read of size 8 at addr ffffffc324e78618 by task systemd-udevd/312 >> >> Allocated by task 312: >> >> Freed by task 312: >> >> The buggy address belongs to the object at ffffffc324e78600 >> which belongs to the cache kmalloc-64 of size 64 The buggy address >> is located 24 bytes inside of >> 64-byte region [ffffffc324e78600, ffffffc324e78640) The buggy >> address belongs to the page: >> page:ffffffbf0c939e00 count:1 mapcount:0 mapping: >> (null) index:0xffffffc324e78f80 >> flags: 0x4000000000000100(slab) >> raw: 4000000000000100 0000000000000000 ffffffc324e78f80 >> 000000018020001a >> raw: 0000000000000000 0000000100000001 ffffffc37040fb80 >> 0000000000000000 page dumped because: kasan: bad access detected >> >> Memory state around the buggy address: >> ffffffc324e78500: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc >> ffffffc324e78580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc >> >ffffffc324e78600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc >> ^ >> ffffffc324e78680: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc >> ffffffc324e78700: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc >> ================================================================== >> >> Suggested-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> >> Signed-off-by: Sumit Gupta <sumitg@xxxxxxxxxx> >> --- >> v2: >> * update ctrl->cluster only when new control reference is added. >> * check ctrl->ncontrols to avoid illegal access when cluster has zero controls. >> >> drivers/media/v4l2-core/v4l2-ctrls.c | 53 >> +++++++++++++++++++----------------- >> 1 file changed, 28 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c >> b/drivers/media/v4l2-core/v4l2-ctrls.c >> index 5e3806f..3d0a952 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >> @@ -2154,15 +2154,6 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl, >> if (size_extra_req) >> new_ref->p_req.p = &new_ref[1]; >> >> - if (ctrl->handler == hdl) { >> - /* By default each control starts in a cluster of its own. >> - new_ref->ctrl is basically a cluster array with one >> - element, so that's perfect to use as the cluster pointer. >> - But only do this for the handler that owns the control. */ >> - ctrl->cluster = &new_ref->ctrl; >> - ctrl->ncontrols = 1; >> - } >> - >> INIT_LIST_HEAD(&new_ref->node); >> >> mutex_lock(hdl->lock); >> @@ -2195,6 +2186,15 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl, >> hdl->buckets[bucket] = new_ref; >> if (ctrl_ref) >> *ctrl_ref = new_ref; >> + if (ctrl->handler == hdl) { >> + /* By default each control starts in a cluster of its own. >> + * new_ref->ctrl is basically a cluster array with one >> + * element, so that's perfect to use as the cluster pointer. >> + * But only do this for the handler that owns the control. >> + */ >> + ctrl->cluster = &new_ref->ctrl; >> + ctrl->ncontrols = 1; >> + } >> >> unlock: >> mutex_unlock(hdl->lock); > > This looks good. > >> @@ -2725,25 +2725,28 @@ int __v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl) >> ctrl->done = false; >> >> list_for_each_entry(ctrl, &hdl->ctrls, node) { >> - struct v4l2_ctrl *master = ctrl->cluster[0]; >> - int i; >> - >> - /* Skip if this control was already handled by a cluster. */ >> - /* Skip button controls and read-only controls. */ >> - if (ctrl->done || ctrl->type == V4L2_CTRL_TYPE_BUTTON || >> - (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)) >> - continue; >> + if (ctrl->ncontrols) { >> + struct v4l2_ctrl *master = ctrl->cluster[0]; >> + int i; >> + >> + /* Skip if this control was already handled by a */ >> + /* cluster. Skip button controls and read-only */ >> + /* controls. */ >> + if (ctrl->done || ctrl->type == V4L2_CTRL_TYPE_BUTTON || >> + (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)) >> + continue; >> >> - for (i = 0; i < master->ncontrols; i++) { >> - if (master->cluster[i]) { >> - cur_to_new(master->cluster[i]); >> - master->cluster[i]->is_new = 1; >> - master->cluster[i]->done = true; >> + for (i = 0; i < master->ncontrols; i++) { >> + if (master->cluster[i]) { >> + cur_to_new(master->cluster[i]); >> + master->cluster[i]->is_new = 1; >> + master->cluster[i]->done = true; >> + } >> } >> + ret = call_op(master, s_ctrl); >> + if (ret) >> + break; >> } >> - ret = call_op(master, s_ctrl); >> - if (ret) >> - break; >> } >> >> return ret; >> > > These changes shouldn't be needed: when you get here all controls are guaranteed to have a non-0 ncontrols value. > > If it is possible to encounter a control where ncontrols == 0, then that's a bug elsewhere. But as far as I can tell that isn't the case. > > Regards, > > Hans > > Yes, I am getting a control where ncontrols is zero as printed below. > It can be another bug as you said. I think, adding check here will be > helpful to not crash in such cases. Please share if you think otherwise, > then will remove the check. > > int __v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl) > ctrl->done = false; > > list_for_each_entry(ctrl, &hdl->ctrls, node) { > + if (ctrl->ncontrols == 0) > + pr_crit("<func:%s><ctrl->name:%s><ctrl->ncontrols:%d>\n", > + __func__, ctrl->name, ctrl->ncontrols); > > Log: > [ 8.354464] <func:__v4l2_ctrl_handler_setup><ctrl->name:Group Hold><ctrl->ncontrols:0> Something went wrong if you get this. This should never happen. Dig a bit deeper to find out how you get into this situation. You did check hdl->error before calling v4l2_ctrl_handler_setup? Although even if an error occurred when adding controls, this should still not happen. Regards, Hans > > > Regards, > Sumit Gupta >