| Message ID | ddb6e006-7440-41c5-8aaa-685b058418b4@xs4all.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] media: v4l2-core: v4l2-ctrls: check for handler_new_ref misuse | expand |
Hi Hans, On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote: > An out-of-tree driver created a control handler, added controls, then > called v4l2_ctrl_add_handler to add references to controls from another > handler, and finally added another control that happened to have the same > control ID as one of the controls from that other handler. > > This caused a NULL pointer crash when an attempt was made to use that last > control. > > Besides the fact that two out-of-tree drivers used the same control ID for > different (private) controls, which is obviously a bug, this specific > scenario should have been caught. The root cause is the 'duplicate ID' > check in handler_new_ref(): it expects that drivers will first add all > controls to a control handler before calling v4l2_ctrl_add_handler. That > way the local controls will always override references to controls from > another handler. Do we support adding new controls after adding the handler or is there a valid use case for it? I'd rather say it's not supported and prevent it, for simplicity. Things like this will likely make it more difficult to move the controls to the device state. Cc Laurent and Jacopo. > > It never considered the case where new local controls were added after > calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return > an error in the case that a new local control is added with the same ID as > an existing control reference. Also use WARN_ON since this is a driver bug. > > This situation can only happen when out-of-tree drivers are used or during > driver development, since mainlined drivers all have their own control > ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. > > Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > --- > Changes since v1: > Improved the comment. > --- > drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > index eeab6a5eb7ba..8fac12e78481 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; > int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ > unsigned int size_extra_req = 0; > + int ret = 0; > > if (ctrl_ref) > *ctrl_ref = NULL; > @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > list_for_each_entry(ref, &hdl->ctrl_refs, node) { > if (ref->ctrl->id < id) > continue; > - /* Don't add duplicates */ > - if (ref->ctrl->id == id) { > - kfree(new_ref); > - goto unlock; > + /* Check we're not adding a duplicate */ > + if (ref->ctrl->id != id) { > + list_add(&new_ref->node, ref->node.prev); > + break; > } > - list_add(&new_ref->node, ref->node.prev); > - break; > + > + /* > + * If we add a new control to this control handler, and we find > + * that it is a duplicate, then that is a driver bug. Warn and > + * return an error. > + * > + * It can be caused by either adding the same control twice, or > + * by first calling v4l2_ctrl_add_handler, and then adding a new > + * control to this control handler. > + * > + * Either sequence is incorrect. > + * > + * However, if the control is owned by another handler, and > + * a control with that ID already exists in the list, then we > + * can safely skip it: in that case it the control is overridden > + * by the existing control. > + */ > + if (WARN_ON(hdl == ctrl->handler)) > + ret = -EEXIST; > + kfree(new_ref); > + goto unlock; > } > > insert_in_hash: > @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > > unlock: > mutex_unlock(hdl->lock); > + if (ret) > + return handler_set_err(hdl, ret); > return 0; > } >
On 08/11/2024 11:15, Sakari Ailus wrote: > Hi Hans, > > On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote: >> An out-of-tree driver created a control handler, added controls, then >> called v4l2_ctrl_add_handler to add references to controls from another >> handler, and finally added another control that happened to have the same >> control ID as one of the controls from that other handler. >> >> This caused a NULL pointer crash when an attempt was made to use that last >> control. >> >> Besides the fact that two out-of-tree drivers used the same control ID for >> different (private) controls, which is obviously a bug, this specific >> scenario should have been caught. The root cause is the 'duplicate ID' >> check in handler_new_ref(): it expects that drivers will first add all >> controls to a control handler before calling v4l2_ctrl_add_handler. That >> way the local controls will always override references to controls from >> another handler. > > Do we support adding new controls after adding the handler or is there a > valid use case for it? I'd rather say it's not supported and prevent it, > for simplicity. Things like this will likely make it more difficult to move > the controls to the device state. Blocking this completely is out of scope of this patch. I am not quite sure if doing that wouldn't break some drivers (in or out of tree). If this turns out to be an issue when moving controls to the device state, then we can revisit this. Regards, Hans > > Cc Laurent and Jacopo. > >> >> It never considered the case where new local controls were added after >> calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return >> an error in the case that a new local control is added with the same ID as >> an existing control reference. Also use WARN_ON since this is a driver bug. >> >> This situation can only happen when out-of-tree drivers are used or during >> driver development, since mainlined drivers all have their own control >> ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. >> >> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> >> --- >> Changes since v1: >> Improved the comment. >> --- >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- >> 1 file changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c >> index eeab6a5eb7ba..8fac12e78481 100644 >> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c >> @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >> u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; >> int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ >> unsigned int size_extra_req = 0; >> + int ret = 0; >> >> if (ctrl_ref) >> *ctrl_ref = NULL; >> @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >> list_for_each_entry(ref, &hdl->ctrl_refs, node) { >> if (ref->ctrl->id < id) >> continue; >> - /* Don't add duplicates */ >> - if (ref->ctrl->id == id) { >> - kfree(new_ref); >> - goto unlock; >> + /* Check we're not adding a duplicate */ >> + if (ref->ctrl->id != id) { >> + list_add(&new_ref->node, ref->node.prev); >> + break; >> } >> - list_add(&new_ref->node, ref->node.prev); >> - break; >> + >> + /* >> + * If we add a new control to this control handler, and we find >> + * that it is a duplicate, then that is a driver bug. Warn and >> + * return an error. >> + * >> + * It can be caused by either adding the same control twice, or >> + * by first calling v4l2_ctrl_add_handler, and then adding a new >> + * control to this control handler. >> + * >> + * Either sequence is incorrect. >> + * >> + * However, if the control is owned by another handler, and >> + * a control with that ID already exists in the list, then we >> + * can safely skip it: in that case it the control is overridden >> + * by the existing control. >> + */ >> + if (WARN_ON(hdl == ctrl->handler)) >> + ret = -EEXIST; >> + kfree(new_ref); >> + goto unlock; >> } >> >> insert_in_hash: >> @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >> >> unlock: >> mutex_unlock(hdl->lock); >> + if (ret) >> + return handler_set_err(hdl, ret); >> return 0; >> } >> >
Hello, On Fri, Nov 08, 2024 at 11:49:01AM +0100, Hans Verkuil wrote: > On 08/11/2024 11:15, Sakari Ailus wrote: > > On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote: > >> An out-of-tree driver created a control handler, added controls, then > >> called v4l2_ctrl_add_handler to add references to controls from another > >> handler, and finally added another control that happened to have the same > >> control ID as one of the controls from that other handler. Naughty driver :-) > >> > >> This caused a NULL pointer crash when an attempt was made to use that last > >> control. > >> > >> Besides the fact that two out-of-tree drivers used the same control ID for > >> different (private) controls, which is obviously a bug, this specific > >> scenario should have been caught. The root cause is the 'duplicate ID' > >> check in handler_new_ref(): it expects that drivers will first add all > >> controls to a control handler before calling v4l2_ctrl_add_handler. That > >> way the local controls will always override references to controls from > >> another handler. > > > > Do we support adding new controls after adding the handler or is there a > > valid use case for it? I'd rather say it's not supported and prevent it, > > for simplicity. Things like this will likely make it more difficult to move > > the controls to the device state. > > Blocking this completely is out of scope of this patch. I am not quite sure > if doing that wouldn't break some drivers (in or out of tree). > > If this turns out to be an issue when moving controls to the device state, > then we can revisit this. I tend to agree with Sakari here. I believe the control framework is already complex enough, and I don't think we should allow drivers to add cnotrols after calling v4l2_ctrl_add_handler(). If there are any in-tree drivers doing so, we can probably fix them fairly easily. As for generating a warning instead of crashing when the control is accessed, we could generate a warning if a control is added by the driver after calling v4l2_ctrl_add_handler(). That could even cause the control handler to flag an error, and that would be very visible to driver authors. > > Cc Laurent and Jacopo. > > > >> It never considered the case where new local controls were added after > >> calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return > >> an error in the case that a new local control is added with the same ID as > >> an existing control reference. Also use WARN_ON since this is a driver bug. > >> > >> This situation can only happen when out-of-tree drivers are used or during > >> driver development, since mainlined drivers all have their own control > >> ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. > >> > >> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > >> --- > >> Changes since v1: > >> Improved the comment. > >> --- > >> drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- > >> 1 file changed, 28 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >> index eeab6a5eb7ba..8fac12e78481 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > >> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >> @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >> u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; > >> int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ > >> unsigned int size_extra_req = 0; > >> + int ret = 0; > >> > >> if (ctrl_ref) > >> *ctrl_ref = NULL; > >> @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >> list_for_each_entry(ref, &hdl->ctrl_refs, node) { > >> if (ref->ctrl->id < id) > >> continue; > >> - /* Don't add duplicates */ > >> - if (ref->ctrl->id == id) { > >> - kfree(new_ref); > >> - goto unlock; > >> + /* Check we're not adding a duplicate */ > >> + if (ref->ctrl->id != id) { > >> + list_add(&new_ref->node, ref->node.prev); > >> + break; > >> } > >> - list_add(&new_ref->node, ref->node.prev); > >> - break; > >> + > >> + /* > >> + * If we add a new control to this control handler, and we find > >> + * that it is a duplicate, then that is a driver bug. Warn and > >> + * return an error. > >> + * > >> + * It can be caused by either adding the same control twice, or > >> + * by first calling v4l2_ctrl_add_handler, and then adding a new > >> + * control to this control handler. > >> + * > >> + * Either sequence is incorrect. > >> + * > >> + * However, if the control is owned by another handler, and > >> + * a control with that ID already exists in the list, then we > >> + * can safely skip it: in that case it the control is overridden > >> + * by the existing control. > >> + */ > >> + if (WARN_ON(hdl == ctrl->handler)) > >> + ret = -EEXIST; > >> + kfree(new_ref); > >> + goto unlock; > >> } > >> > >> insert_in_hash: > >> @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >> > >> unlock: > >> mutex_unlock(hdl->lock); > >> + if (ret) > >> + return handler_set_err(hdl, ret); > >> return 0; > >> } > >>
Hi Laurent, On 08/11/2024 14:28, Laurent Pinchart wrote: > Hello, > > On Fri, Nov 08, 2024 at 11:49:01AM +0100, Hans Verkuil wrote: >> On 08/11/2024 11:15, Sakari Ailus wrote: >>> On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote: >>>> An out-of-tree driver created a control handler, added controls, then >>>> called v4l2_ctrl_add_handler to add references to controls from another >>>> handler, and finally added another control that happened to have the same >>>> control ID as one of the controls from that other handler. > > Naughty driver :-) > >>>> >>>> This caused a NULL pointer crash when an attempt was made to use that last >>>> control. >>>> >>>> Besides the fact that two out-of-tree drivers used the same control ID for >>>> different (private) controls, which is obviously a bug, this specific >>>> scenario should have been caught. The root cause is the 'duplicate ID' >>>> check in handler_new_ref(): it expects that drivers will first add all >>>> controls to a control handler before calling v4l2_ctrl_add_handler. That >>>> way the local controls will always override references to controls from >>>> another handler. >>> >>> Do we support adding new controls after adding the handler or is there a >>> valid use case for it? I'd rather say it's not supported and prevent it, >>> for simplicity. Things like this will likely make it more difficult to move >>> the controls to the device state. >> >> Blocking this completely is out of scope of this patch. I am not quite sure >> if doing that wouldn't break some drivers (in or out of tree). >> >> If this turns out to be an issue when moving controls to the device state, >> then we can revisit this. > > I tend to agree with Sakari here. I believe the control framework is > already complex enough, and I don't think we should allow drivers to add > cnotrols after calling v4l2_ctrl_add_handler(). If there are any in-tree > drivers doing so, we can probably fix them fairly easily. > > As for generating a warning instead of crashing when the control is > accessed, we could generate a warning if a control is added by the > driver after calling v4l2_ctrl_add_handler(). That could even cause the > control handler to flag an error, and that would be very visible to > driver authors. While I agree with this, I don't want to do this without first doing some analysis for existing drivers. Would you be OK with me merging this patch, and that I do the analysis later and post a follow-up patch? This issue causes a somewhat hard-to-find crash and it hit me twice within a week. Regards, Hans > >>> Cc Laurent and Jacopo. >>> >>>> It never considered the case where new local controls were added after >>>> calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return >>>> an error in the case that a new local control is added with the same ID as >>>> an existing control reference. Also use WARN_ON since this is a driver bug. >>>> >>>> This situation can only happen when out-of-tree drivers are used or during >>>> driver development, since mainlined drivers all have their own control >>>> ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. >>>> >>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> >>>> --- >>>> Changes since v1: >>>> Improved the comment. >>>> --- >>>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- >>>> 1 file changed, 28 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c >>>> index eeab6a5eb7ba..8fac12e78481 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c >>>> @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >>>> u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; >>>> int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ >>>> unsigned int size_extra_req = 0; >>>> + int ret = 0; >>>> >>>> if (ctrl_ref) >>>> *ctrl_ref = NULL; >>>> @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >>>> list_for_each_entry(ref, &hdl->ctrl_refs, node) { >>>> if (ref->ctrl->id < id) >>>> continue; >>>> - /* Don't add duplicates */ >>>> - if (ref->ctrl->id == id) { >>>> - kfree(new_ref); >>>> - goto unlock; >>>> + /* Check we're not adding a duplicate */ >>>> + if (ref->ctrl->id != id) { >>>> + list_add(&new_ref->node, ref->node.prev); >>>> + break; >>>> } >>>> - list_add(&new_ref->node, ref->node.prev); >>>> - break; >>>> + >>>> + /* >>>> + * If we add a new control to this control handler, and we find >>>> + * that it is a duplicate, then that is a driver bug. Warn and >>>> + * return an error. >>>> + * >>>> + * It can be caused by either adding the same control twice, or >>>> + * by first calling v4l2_ctrl_add_handler, and then adding a new >>>> + * control to this control handler. >>>> + * >>>> + * Either sequence is incorrect. >>>> + * >>>> + * However, if the control is owned by another handler, and >>>> + * a control with that ID already exists in the list, then we >>>> + * can safely skip it: in that case it the control is overridden >>>> + * by the existing control. >>>> + */ >>>> + if (WARN_ON(hdl == ctrl->handler)) >>>> + ret = -EEXIST; >>>> + kfree(new_ref); >>>> + goto unlock; >>>> } >>>> >>>> insert_in_hash: >>>> @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, >>>> >>>> unlock: >>>> mutex_unlock(hdl->lock); >>>> + if (ret) >>>> + return handler_set_err(hdl, ret); >>>> return 0; >>>> } >>>> >
On Fri, Nov 08, 2024 at 03:38:27PM +0100, Hans Verkuil wrote: > On 08/11/2024 14:28, Laurent Pinchart wrote: > > On Fri, Nov 08, 2024 at 11:49:01AM +0100, Hans Verkuil wrote: > >> On 08/11/2024 11:15, Sakari Ailus wrote: > >>> On Tue, Nov 05, 2024 at 08:42:04AM +0100, Hans Verkuil wrote: > >>>> An out-of-tree driver created a control handler, added controls, then > >>>> called v4l2_ctrl_add_handler to add references to controls from another > >>>> handler, and finally added another control that happened to have the same > >>>> control ID as one of the controls from that other handler. > > > > Naughty driver :-) > > > >>>> > >>>> This caused a NULL pointer crash when an attempt was made to use that last > >>>> control. > >>>> > >>>> Besides the fact that two out-of-tree drivers used the same control ID for > >>>> different (private) controls, which is obviously a bug, this specific > >>>> scenario should have been caught. The root cause is the 'duplicate ID' > >>>> check in handler_new_ref(): it expects that drivers will first add all > >>>> controls to a control handler before calling v4l2_ctrl_add_handler. That > >>>> way the local controls will always override references to controls from > >>>> another handler. > >>> > >>> Do we support adding new controls after adding the handler or is there a > >>> valid use case for it? I'd rather say it's not supported and prevent it, > >>> for simplicity. Things like this will likely make it more difficult to move > >>> the controls to the device state. > >> > >> Blocking this completely is out of scope of this patch. I am not quite sure > >> if doing that wouldn't break some drivers (in or out of tree). > >> > >> If this turns out to be an issue when moving controls to the device state, > >> then we can revisit this. > > > > I tend to agree with Sakari here. I believe the control framework is > > already complex enough, and I don't think we should allow drivers to add > > cnotrols after calling v4l2_ctrl_add_handler(). If there are any in-tree > > drivers doing so, we can probably fix them fairly easily. > > > > As for generating a warning instead of crashing when the control is > > accessed, we could generate a warning if a control is added by the > > driver after calling v4l2_ctrl_add_handler(). That could even cause the > > control handler to flag an error, and that would be very visible to > > driver authors. > > While I agree with this, I don't want to do this without first doing > some analysis for existing drivers. Better safe than sorry, sure. > Would you be OK with me merging this patch, and that I do the analysis later > and post a follow-up patch? I'm OK with that. Could you then mention in the comment that adding controls after calling v4l2_ctrl_add_handler() isn't allowed ? That will make me feel better about people not getting the wrong impression. > This issue causes a somewhat hard-to-find crash and it hit me twice > within a week. > > >>> Cc Laurent and Jacopo. > >>> > >>>> It never considered the case where new local controls were added after > >>>> calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return > >>>> an error in the case that a new local control is added with the same ID as > >>>> an existing control reference. Also use WARN_ON since this is a driver bug. > >>>> > >>>> This situation can only happen when out-of-tree drivers are used or during > >>>> driver development, since mainlined drivers all have their own control > >>>> ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. > >>>> > >>>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> > >>>> --- > >>>> Changes since v1: > >>>> Improved the comment. > >>>> --- > >>>> drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- > >>>> 1 file changed, 28 insertions(+), 6 deletions(-) > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>>> index eeab6a5eb7ba..8fac12e78481 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > >>>> @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >>>> u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; > >>>> int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ > >>>> unsigned int size_extra_req = 0; > >>>> + int ret = 0; > >>>> > >>>> if (ctrl_ref) > >>>> *ctrl_ref = NULL; > >>>> @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >>>> list_for_each_entry(ref, &hdl->ctrl_refs, node) { > >>>> if (ref->ctrl->id < id) > >>>> continue; > >>>> - /* Don't add duplicates */ > >>>> - if (ref->ctrl->id == id) { > >>>> - kfree(new_ref); > >>>> - goto unlock; > >>>> + /* Check we're not adding a duplicate */ > >>>> + if (ref->ctrl->id != id) { > >>>> + list_add(&new_ref->node, ref->node.prev); > >>>> + break; > >>>> } > >>>> - list_add(&new_ref->node, ref->node.prev); > >>>> - break; > >>>> + > >>>> + /* > >>>> + * If we add a new control to this control handler, and we find > >>>> + * that it is a duplicate, then that is a driver bug. Warn and > >>>> + * return an error. > >>>> + * > >>>> + * It can be caused by either adding the same control twice, or > >>>> + * by first calling v4l2_ctrl_add_handler, and then adding a new > >>>> + * control to this control handler. > >>>> + * > >>>> + * Either sequence is incorrect. > >>>> + * > >>>> + * However, if the control is owned by another handler, and > >>>> + * a control with that ID already exists in the list, then we > >>>> + * can safely skip it: in that case it the control is overridden > >>>> + * by the existing control. > >>>> + */ > >>>> + if (WARN_ON(hdl == ctrl->handler)) > >>>> + ret = -EEXIST; > >>>> + kfree(new_ref); > >>>> + goto unlock; > >>>> } > >>>> > >>>> insert_in_hash: > >>>> @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, > >>>> > >>>> unlock: > >>>> mutex_unlock(hdl->lock); > >>>> + if (ret) > >>>> + return handler_set_err(hdl, ret); > >>>> return 0; > >>>> } > >>>>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c index eeab6a5eb7ba..8fac12e78481 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c @@ -1676,6 +1676,7 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1; int bucket = id % hdl->nr_of_buckets; /* which bucket to use */ unsigned int size_extra_req = 0; + int ret = 0; if (ctrl_ref) *ctrl_ref = NULL; @@ -1719,13 +1720,32 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, list_for_each_entry(ref, &hdl->ctrl_refs, node) { if (ref->ctrl->id < id) continue; - /* Don't add duplicates */ - if (ref->ctrl->id == id) { - kfree(new_ref); - goto unlock; + /* Check we're not adding a duplicate */ + if (ref->ctrl->id != id) { + list_add(&new_ref->node, ref->node.prev); + break; } - list_add(&new_ref->node, ref->node.prev); - break; + + /* + * If we add a new control to this control handler, and we find + * that it is a duplicate, then that is a driver bug. Warn and + * return an error. + * + * It can be caused by either adding the same control twice, or + * by first calling v4l2_ctrl_add_handler, and then adding a new + * control to this control handler. + * + * Either sequence is incorrect. + * + * However, if the control is owned by another handler, and + * a control with that ID already exists in the list, then we + * can safely skip it: in that case it the control is overridden + * by the existing control. + */ + if (WARN_ON(hdl == ctrl->handler)) + ret = -EEXIST; + kfree(new_ref); + goto unlock; } insert_in_hash: @@ -1746,6 +1766,8 @@ int handler_new_ref(struct v4l2_ctrl_handler *hdl, unlock: mutex_unlock(hdl->lock); + if (ret) + return handler_set_err(hdl, ret); return 0; }
An out-of-tree driver created a control handler, added controls, then called v4l2_ctrl_add_handler to add references to controls from another handler, and finally added another control that happened to have the same control ID as one of the controls from that other handler. This caused a NULL pointer crash when an attempt was made to use that last control. Besides the fact that two out-of-tree drivers used the same control ID for different (private) controls, which is obviously a bug, this specific scenario should have been caught. The root cause is the 'duplicate ID' check in handler_new_ref(): it expects that drivers will first add all controls to a control handler before calling v4l2_ctrl_add_handler. That way the local controls will always override references to controls from another handler. It never considered the case where new local controls were added after calling v4l2_ctrl_add_handler. Add a check to handler_new_ref() to return an error in the case that a new local control is added with the same ID as an existing control reference. Also use WARN_ON since this is a driver bug. This situation can only happen when out-of-tree drivers are used or during driver development, since mainlined drivers all have their own control ranges reserved in v4l2-controls.h, thus preventing duplicate control IDs. Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl> --- Changes since v1: Improved the comment. --- drivers/media/v4l2-core/v4l2-ctrls-core.c | 34 +++++++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-)