| Message ID | 20251202-cam-control-override-v3-5-eacab052798d@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo On 02/12/2025 14:49, Jacopo Mondi wrote: > The list of controls part of a Request is initialized with the > ControlInfoMap of the Camera the Request is created from. > > Applications can re-assign the controls list in a Request > which could cause issues during serialization. > > Validate that the ControlList in a Request is valid when > the Request is queued to the Camera by inspecting the > ControlInfoMap pointer. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/camera.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) > return -EINVAL; > } > > + /* Make sure the Request has a valid control list. */ > + if (request->controls().infoMap() != &controls()) { > + LOG(Camera, Error) << "Invalid control list in the Request"; > + return -EINVAL; > + } > + So this is a pointer comparison rather than a _contents_ comparison right? Could we not prevent the pointer from being overwritten? Thanks Dan > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle >
Hi Dan On Mon, Dec 15, 2025 at 12:21:17PM +0000, Dan Scally wrote: > Hi Jacopo > > On 02/12/2025 14:49, Jacopo Mondi wrote: > > The list of controls part of a Request is initialized with the > > ControlInfoMap of the Camera the Request is created from. > > > > Applications can re-assign the controls list in a Request > > which could cause issues during serialization. > > > > Validate that the ControlList in a Request is valid when > > the Request is queued to the Camera by inspecting the > > ControlInfoMap pointer. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/libcamera/camera.cpp | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) > > return -EINVAL; > > } > > + /* Make sure the Request has a valid control list. */ > > + if (request->controls().infoMap() != &controls()) { > > + LOG(Camera, Error) << "Invalid control list in the Request"; > > + return -EINVAL; > > + } > > + > > So this is a pointer comparison rather than a _contents_ comparison right? Yes > Could we not prevent the pointer from being overwritten? I tried, by - ControlList &controls() { return *controls_; } + const ControlList &controls() const { return *controls_; } https://patchwork.libcamera.org/patch/25212/ > > Thanks > Dan > > > /* > > * The camera state may change until the end of the function. No locking > > * is however needed as PipelineHandler::queueRequest() will handle > > >
Hi Jacopo On 15/12/2025 12:30, Jacopo Mondi wrote: > Hi Dan > > On Mon, Dec 15, 2025 at 12:21:17PM +0000, Dan Scally wrote: >> Hi Jacopo >> >> On 02/12/2025 14:49, Jacopo Mondi wrote: >>> The list of controls part of a Request is initialized with the >>> ControlInfoMap of the Camera the Request is created from. >>> >>> Applications can re-assign the controls list in a Request >>> which could cause issues during serialization. >>> >>> Validate that the ControlList in a Request is valid when >>> the Request is queued to the Camera by inspecting the >>> ControlInfoMap pointer. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >>> src/libcamera/camera.cpp | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>> index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 >>> --- a/src/libcamera/camera.cpp >>> +++ b/src/libcamera/camera.cpp >>> @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) >>> return -EINVAL; >>> } >>> + /* Make sure the Request has a valid control list. */ >>> + if (request->controls().infoMap() != &controls()) { >>> + LOG(Camera, Error) << "Invalid control list in the Request"; >>> + return -EINVAL; >>> + } >>> + >> >> So this is a pointer comparison rather than a _contents_ comparison right? > > Yes > >> Could we not prevent the pointer from being overwritten? > > I tried, by > - ControlList &controls() { return *controls_; } > + const ControlList &controls() const { return *controls_; } > > https://patchwork.libcamera.org/patch/25212/ Ah-ha...ok. Hmm, I feel like this is the same change by proxy, really. If the goal is to make sure that the Request is only queued with controls that are valid for the Camera, what about something like: /* Make sure the Request has a valid control list. */ for (auto control : request->controls()) { if (!d->validator()->validate(control.first)) { LOG(Camera, Error) << "Invalid control in Request"; return -EINVAL; } } The Request's ControlList is created with the Camera's validator by default, so unless the application has replaced the pointer validation should work here too. > > >> >> Thanks >> Dan >> >>> /* >>> * The camera state may change until the end of the function. No locking >>> * is however needed as PipelineHandler::queueRequest() will handle >>> >>
Hi Dan On Mon, Dec 15, 2025 at 02:08:10PM +0000, Dan Scally wrote: > Hi Jacopo > > On 15/12/2025 12:30, Jacopo Mondi wrote: > > Hi Dan > > > > On Mon, Dec 15, 2025 at 12:21:17PM +0000, Dan Scally wrote: > > > Hi Jacopo > > > > > > On 02/12/2025 14:49, Jacopo Mondi wrote: > > > > The list of controls part of a Request is initialized with the > > > > ControlInfoMap of the Camera the Request is created from. > > > > > > > > Applications can re-assign the controls list in a Request > > > > which could cause issues during serialization. > > > > > > > > Validate that the ControlList in a Request is valid when > > > > the Request is queued to the Camera by inspecting the > > > > ControlInfoMap pointer. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/libcamera/camera.cpp | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) > > > > return -EINVAL; > > > > } > > > > + /* Make sure the Request has a valid control list. */ > > > > + if (request->controls().infoMap() != &controls()) { > > > > + LOG(Camera, Error) << "Invalid control list in the Request"; > > > > + return -EINVAL; > > > > + } > > > > + > > > > > > So this is a pointer comparison rather than a _contents_ comparison right? > > > > Yes > > > > > Could we not prevent the pointer from being overwritten? > > > > I tried, by > > - ControlList &controls() { return *controls_; } > > + const ControlList &controls() const { return *controls_; } > > > > https://patchwork.libcamera.org/patch/25212/ > > Ah-ha...ok. Hmm, I feel like this is the same change by proxy, really. If > the goal is to make sure that the Request is only queued with controls that > are valid for the Camera, what about something like: The purpose is for application not to override the control list in a request, as they can currently do request->controls() = { anything } and that's what 'cam' was doing and breaking serialization as a consequence. > > /* Make sure the Request has a valid control list. */ > for (auto control : request->controls()) { > if (!d->validator()->validate(control.first)) { > LOG(Camera, Error) << "Invalid control in Request"; > return -EINVAL; > } > } A request control list is already created with the camera control validator controls_ = new ControlList(controls::controls, camera->_d()->validator()); and validator->validate() is already called everytime a control is looked up ControlValue *ControlList::find(unsigned int id) { if (validator_ && !validator_->validate(id)) { LOG(Controls, Error) << "Control " << utils::hex(id) << " is not valid for " << validator_->name(); return nullptr; } > > The Request's ControlList is created with the Camera's validator by default, > so unless the application has replaced the pointer validation should work > here too. Exactly, applications can overwrite the request->controls() list. If they populate the new list with valid controls, the above check will pass, but the control list could not have a valid control info map. I agree what's implemented here is a minimal protection, I would have liked to go stricter and remove rw access to Request::controls() and go through an API to set controls in a request, but as that wasn't apreciated I think that's the minimum we should do. > > > > > > > > > > > > > Thanks > > > Dan > > > > > > > /* > > > > * The camera state may change until the end of the function. No locking > > > > * is however needed as PipelineHandler::queueRequest() will handle > > > > > > > >
Hi Jacopo On 15/12/2025 15:49, Jacopo Mondi wrote: > Hi Dan > > On Mon, Dec 15, 2025 at 02:08:10PM +0000, Dan Scally wrote: >> Hi Jacopo >> >> On 15/12/2025 12:30, Jacopo Mondi wrote: >>> Hi Dan >>> >>> On Mon, Dec 15, 2025 at 12:21:17PM +0000, Dan Scally wrote: >>>> Hi Jacopo >>>> >>>> On 02/12/2025 14:49, Jacopo Mondi wrote: >>>>> The list of controls part of a Request is initialized with the >>>>> ControlInfoMap of the Camera the Request is created from. >>>>> >>>>> Applications can re-assign the controls list in a Request >>>>> which could cause issues during serialization. >>>>> >>>>> Validate that the ControlList in a Request is valid when >>>>> the Request is queued to the Camera by inspecting the >>>>> ControlInfoMap pointer. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>> --- >>>>> src/libcamera/camera.cpp | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >>>>> index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 >>>>> --- a/src/libcamera/camera.cpp >>>>> +++ b/src/libcamera/camera.cpp >>>>> @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) >>>>> return -EINVAL; >>>>> } >>>>> + /* Make sure the Request has a valid control list. */ >>>>> + if (request->controls().infoMap() != &controls()) { >>>>> + LOG(Camera, Error) << "Invalid control list in the Request"; >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>> >>>> So this is a pointer comparison rather than a _contents_ comparison right? >>> >>> Yes >>> >>>> Could we not prevent the pointer from being overwritten? >>> >>> I tried, by >>> - ControlList &controls() { return *controls_; } >>> + const ControlList &controls() const { return *controls_; } >>> >>> https://patchwork.libcamera.org/patch/25212/ >> >> Ah-ha...ok. Hmm, I feel like this is the same change by proxy, really. If >> the goal is to make sure that the Request is only queued with controls that >> are valid for the Camera, what about something like: > > The purpose is for application not to override the control list in a > request, as they can currently do > > request->controls() = { anything } > > and that's what 'cam' was doing and breaking serialization as a > consequence. > >> >> /* Make sure the Request has a valid control list. */ >> for (auto control : request->controls()) { >> if (!d->validator()->validate(control.first)) { >> LOG(Camera, Error) << "Invalid control in Request"; >> return -EINVAL; >> } >> } > > A request control list is already created with the camera control > validator > > controls_ = new ControlList(controls::controls, > camera->_d()->validator()); > > and validator->validate() is already called everytime a control is > looked up > > ControlValue *ControlList::find(unsigned int id) > { > if (validator_ && !validator_->validate(id)) { > LOG(Controls, Error) > << "Control " << utils::hex(id) > << " is not valid for " << validator_->name(); > return nullptr; > } > >> >> The Request's ControlList is created with the Camera's validator by default, >> so unless the application has replaced the pointer validation should work >> here too. > > Exactly, applications can overwrite the request->controls() list. > > If they populate the new list with valid controls, the above check > will pass, but the control list could not have a valid control info map. > > I agree what's implemented here is a minimal protection, I would have > liked to go stricter and remove rw access to Request::controls() and > go through an API to set controls in a request, but as that wasn't > apreciated I think that's the minimum we should do. Ah - sorry; I should have read the issue from the cover letter and the thread you linked in full before replying. I'm sold now: Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > >> >> >> >>> >>> >>>> >>>> Thanks >>>> Dan >>>> >>>>> /* >>>>> * The camera state may change until the end of the function. No locking >>>>> * is however needed as PipelineHandler::queueRequest() will handle >>>>> >>>> >>
Hi 2025. 12. 02. 15:49 keltezéssel, Jacopo Mondi írta: > The list of controls part of a Request is initialized with the > ControlInfoMap of the Camera the Request is created from. > > Applications can re-assign the controls list in a Request > which could cause issues during serialization. > > Validate that the ControlList in a Request is valid when > the Request is queued to the Camera by inspecting the > ControlInfoMap pointer. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/camera.cpp | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) > return -EINVAL; > } > > + /* Make sure the Request has a valid control list. */ > + if (request->controls().infoMap() != &controls()) { > + LOG(Camera, Error) << "Invalid control list in the Request"; I feel a bit like this message could be more helpful: Request::controls() seems to have been overwritten. This is not allowed. or something like that. Otherwise in my opinion it is somewhat difficult to determine the cause for an app developer. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > + return -EINVAL; > + } > + > /* > * The camera state may change until the end of the function. No locking > * is however needed as PipelineHandler::queueRequest() will handle > > -- > 2.51.1 >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 2e1e146a25b13b94f3a0df5935c0861f78c949ed..93cd4d5f334f982133a12cbc4caec5cdf7bb52c6 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -1345,6 +1345,12 @@ int Camera::queueRequest(Request *request) return -EINVAL; } + /* Make sure the Request has a valid control list. */ + if (request->controls().infoMap() != &controls()) { + LOG(Camera, Error) << "Invalid control list in the Request"; + return -EINVAL; + } + /* * The camera state may change until the end of the function. No locking * is however needed as PipelineHandler::queueRequest() will handle
The list of controls part of a Request is initialized with the ControlInfoMap of the Camera the Request is created from. Applications can re-assign the controls list in a Request which could cause issues during serialization. Validate that the ControlList in a Request is valid when the Request is queued to the Camera by inspecting the ControlInfoMap pointer. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/libcamera/camera.cpp | 6 ++++++ 1 file changed, 6 insertions(+)