| Message ID | 20240827074018.534354-3-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: pisp-be: Split jobs creation and scheduling | expand |
Hi Jacopo, Thank you for the patch. On Tue, Aug 27, 2024 at 09:40:16AM +0200, Jacopo Mondi wrote: > The config parameters buffer is already validated in > pisp_be_validate_config() at .buf_prepare() time. Unfortunately .buf_prepare() isn't the right place to handle the validation. Userspace should not modify the contents of the buffer before BUF_PREPARE and QBUF, but malicious (or just buggy) userspace may. The validation should thus be moved to .buf_queue(). > However some of the same validations are also performed at > pispbe_schedule() time. In particular the function checks that: > > 1) config.num_tiles is valid > 2) At least one of the BAYER or RGB input is enabled > > The input validation is already performed in pisp_be_validate_config() > and there is no need to repeat that at pispbe_schedule() time. Is that the same validation though ? The one in pisp_be_validate_config() validates config->config.global, while the validation in pispbe_schedule() validates job.hw_enables. The latter is set from config->config.global in pispbe_xlate_addrs(), but is later modified in the function. > The num_tiles validation can be moved to pisp_be_validate_config() as > well. As num_tiles is a u32 it can'be be < 0, so change the sanity > check accordingly. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > .../platform/raspberrypi/pisp_be/pisp_be.c | 25 ++++++------------- > 1 file changed, 7 insertions(+), 18 deletions(-) > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > index 8ba1b9f43ba1..73a5c88e25d0 100644 > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > @@ -588,24 +588,6 @@ static void pispbe_schedule(struct pispbe_dev *pispbe, bool clear_hw_busy) > pispbe->hw_busy = true; > spin_unlock_irqrestore(&pispbe->hw_lock, flags); > > - if (job.config->num_tiles <= 0 || > - job.config->num_tiles > PISP_BACK_END_NUM_TILES || > - !((job.hw_enables.bayer_enables | job.hw_enables.rgb_enables) & > - PISP_BE_BAYER_ENABLE_INPUT)) { > - /* > - * Bad job. We can't let it proceed as it could lock up > - * the hardware, or worse! > - * > - * For now, just force num_tiles to 0, which causes the > - * H/W to do something bizarre but survivable. It > - * increments (started,done) counters by more than 1, > - * but we seem to survive... > - */ > - dev_dbg(pispbe->dev, "Bad job: invalid number of tiles: %u\n", > - job.config->num_tiles); > - job.config->num_tiles = 0; > - } > - > pispbe_queue_job(pispbe, &job); > > return; > @@ -703,6 +685,13 @@ static int pisp_be_validate_config(struct pispbe_dev *pispbe, > return -EIO; > } > > + if (config->num_tiles == 0 || > + config->num_tiles > PISP_BACK_END_NUM_TILES) { > + dev_dbg(dev, "%s: Invalid number of tiles: %d\n", __func__, > + config->num_tiles); > + return -EIO; Isn't -EINVAL a better error code ? > + } > + > /* Ensure output config strides and buffer sizes match the V4L2 formats. */ > fmt = &pispbe->node[TDN_OUTPUT_NODE].format; > if (bayer_enables & PISP_BE_BAYER_ENABLE_TDN_OUTPUT) {
Hi Laurent On Sat, Aug 31, 2024 at 04:17:56PM GMT, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Aug 27, 2024 at 09:40:16AM +0200, Jacopo Mondi wrote: > > The config parameters buffer is already validated in > > pisp_be_validate_config() at .buf_prepare() time. > > Unfortunately .buf_prepare() isn't the right place to handle the > validation. Userspace should not modify the contents of the buffer > before BUF_PREPARE and QBUF, but malicious (or just buggy) userspace > may. The validation should thus be moved to .buf_queue(). > Probably right, but unrelated to this patch ? > > However some of the same validations are also performed at > > pispbe_schedule() time. In particular the function checks that: > > > > 1) config.num_tiles is valid > > 2) At least one of the BAYER or RGB input is enabled > > > > The input validation is already performed in pisp_be_validate_config() > > and there is no need to repeat that at pispbe_schedule() time. > > Is that the same validation though ? The one in > pisp_be_validate_config() validates config->config.global, while the > validation in pispbe_schedule() validates job.hw_enables. The latter is > set from config->config.global in pispbe_xlate_addrs(), but is later > modified in the function. > Ah yes, the ones validated at schedule() time are the ones in the job populated by pispbe_xlate_addrs(). However 1) config validation makes sure that in config->config.global enables at least one of BAYER_ENABLE_INPUT or RGB_ENABLE_INPUT is set 2) xlate_addrs() - resets both bayer_enable and rgb_enabl only if there's no main input buffer, which as replied in the previous email, shouldn't happen, otherwise prepare_job() fails before calling xlate_addrs() - set bayer_enable = 0 if the BAYER_ENABLE_INPUT flag wasn't set in config->config.global (in which case rgb_enable is set because of the validation) - clear bit entries in rgb_enable but only for OUTPUTS not for input Which makes me think the validation in schedule() can be removed safely. A bit convoluted, yes, but possibily safe ? > > The num_tiles validation can be moved to pisp_be_validate_config() as > > well. As num_tiles is a u32 it can'be be < 0, so change the sanity > > check accordingly. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > .../platform/raspberrypi/pisp_be/pisp_be.c | 25 ++++++------------- > > 1 file changed, 7 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > index 8ba1b9f43ba1..73a5c88e25d0 100644 > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > @@ -588,24 +588,6 @@ static void pispbe_schedule(struct pispbe_dev *pispbe, bool clear_hw_busy) > > pispbe->hw_busy = true; > > spin_unlock_irqrestore(&pispbe->hw_lock, flags); > > > > - if (job.config->num_tiles <= 0 || > > - job.config->num_tiles > PISP_BACK_END_NUM_TILES || > > - !((job.hw_enables.bayer_enables | job.hw_enables.rgb_enables) & > > - PISP_BE_BAYER_ENABLE_INPUT)) { > > - /* > > - * Bad job. We can't let it proceed as it could lock up > > - * the hardware, or worse! > > - * > > - * For now, just force num_tiles to 0, which causes the > > - * H/W to do something bizarre but survivable. It > > - * increments (started,done) counters by more than 1, > > - * but we seem to survive... > > - */ > > - dev_dbg(pispbe->dev, "Bad job: invalid number of tiles: %u\n", > > - job.config->num_tiles); > > - job.config->num_tiles = 0; > > - } > > - > > pispbe_queue_job(pispbe, &job); > > > > return; > > @@ -703,6 +685,13 @@ static int pisp_be_validate_config(struct pispbe_dev *pispbe, > > return -EIO; > > } > > > > + if (config->num_tiles == 0 || > > + config->num_tiles > PISP_BACK_END_NUM_TILES) { > > + dev_dbg(dev, "%s: Invalid number of tiles: %d\n", __func__, > > + config->num_tiles); > > + return -EIO; > > Isn't -EINVAL a better error code ? > > > + } > > + > > /* Ensure output config strides and buffer sizes match the V4L2 formats. */ > > fmt = &pispbe->node[TDN_OUTPUT_NODE].format; > > if (bayer_enables & PISP_BE_BAYER_ENABLE_TDN_OUTPUT) { > > -- > Regards, > > Laurent Pinchart
On Sat, Aug 31, 2024 at 04:59:32PM +0200, Jacopo Mondi wrote: > On Sat, Aug 31, 2024 at 04:17:56PM GMT, Laurent Pinchart wrote: > > On Tue, Aug 27, 2024 at 09:40:16AM +0200, Jacopo Mondi wrote: > > > The config parameters buffer is already validated in > > > pisp_be_validate_config() at .buf_prepare() time. > > > > Unfortunately .buf_prepare() isn't the right place to handle the > > validation. Userspace should not modify the contents of the buffer > > before BUF_PREPARE and QBUF, but malicious (or just buggy) userspace > > may. The validation should thus be moved to .buf_queue(). > > Probably right, but unrelated to this patch ? Yes, unrelated, but it should be fixed sooner than later as it's a possible security issue. > > > However some of the same validations are also performed at > > > pispbe_schedule() time. In particular the function checks that: > > > > > > 1) config.num_tiles is valid > > > 2) At least one of the BAYER or RGB input is enabled > > > > > > The input validation is already performed in pisp_be_validate_config() > > > and there is no need to repeat that at pispbe_schedule() time. > > > > Is that the same validation though ? The one in > > pisp_be_validate_config() validates config->config.global, while the > > validation in pispbe_schedule() validates job.hw_enables. The latter is > > set from config->config.global in pispbe_xlate_addrs(), but is later > > modified in the function. > > Ah yes, the ones validated at schedule() time are the ones in the job > populated by pispbe_xlate_addrs(). > > However > > 1) config validation makes sure that in config->config.global enables > at least one of BAYER_ENABLE_INPUT or RGB_ENABLE_INPUT is set > > 2) xlate_addrs() > - resets both bayer_enable and rgb_enabl only if > there's no main input buffer, which as replied in the previous > email, shouldn't happen, otherwise prepare_job() fails before > calling xlate_addrs() This is checked in pispbe_xlate_addrs by looking at the return value of pispbe_get_planes_addr() for the main input. That function fails only if (!buf || !node->pisp_format) return 0; buf should indeed not be NULL, as that is checked by pispbe_prepare_job(). node->pisp_format should also never be NULL, as it is initialized at probe time and should never be set to a NULL value afterwards. That part should be fine. I think we should remove the unneeded checks, they only contribute to making the code more convoluted. I'd rather simplify and clarify checks in a single place to give us enough certainty that further checks are not needed. Could you submit follow-up patches for that ? > - set bayer_enable = 0 if the BAYER_ENABLE_INPUT flag wasn't set in > config->config.global (in which case rgb_enable is set because of > the validation) > > - clear bit entries in rgb_enable but only for OUTPUTS not for > input > > > Which makes me think the validation in schedule() can be removed > safely. > > A bit convoluted, yes, but possibily safe ? I think it's safe indeed. But it's definitely too convoluted :-) > > > The num_tiles validation can be moved to pisp_be_validate_config() as > > > well. As num_tiles is a u32 it can'be be < 0, so change the sanity > > > check accordingly. > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > > .../platform/raspberrypi/pisp_be/pisp_be.c | 25 ++++++------------- > > > 1 file changed, 7 insertions(+), 18 deletions(-) > > > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > index 8ba1b9f43ba1..73a5c88e25d0 100644 > > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > @@ -588,24 +588,6 @@ static void pispbe_schedule(struct pispbe_dev *pispbe, bool clear_hw_busy) > > > pispbe->hw_busy = true; > > > spin_unlock_irqrestore(&pispbe->hw_lock, flags); > > > > > > - if (job.config->num_tiles <= 0 || > > > - job.config->num_tiles > PISP_BACK_END_NUM_TILES || > > > - !((job.hw_enables.bayer_enables | job.hw_enables.rgb_enables) & > > > - PISP_BE_BAYER_ENABLE_INPUT)) { > > > - /* > > > - * Bad job. We can't let it proceed as it could lock up > > > - * the hardware, or worse! > > > - * > > > - * For now, just force num_tiles to 0, which causes the > > > - * H/W to do something bizarre but survivable. It > > > - * increments (started,done) counters by more than 1, > > > - * but we seem to survive... > > > - */ > > > - dev_dbg(pispbe->dev, "Bad job: invalid number of tiles: %u\n", > > > - job.config->num_tiles); > > > - job.config->num_tiles = 0; > > > - } > > > - > > > pispbe_queue_job(pispbe, &job); > > > > > > return; > > > @@ -703,6 +685,13 @@ static int pisp_be_validate_config(struct pispbe_dev *pispbe, > > > return -EIO; > > > } > > > > > > + if (config->num_tiles == 0 || > > > + config->num_tiles > PISP_BACK_END_NUM_TILES) { > > > + dev_dbg(dev, "%s: Invalid number of tiles: %d\n", __func__, > > > + config->num_tiles); > > > + return -EIO; > > > > Isn't -EINVAL a better error code ? > > > > > + } > > > + > > > /* Ensure output config strides and buffer sizes match the V4L2 formats. */ > > > fmt = &pispbe->node[TDN_OUTPUT_NODE].format; > > > if (bayer_enables & PISP_BE_BAYER_ENABLE_TDN_OUTPUT) {
Hi Laurent On Tue, Sep 03, 2024 at 02:07:24AM GMT, Laurent Pinchart wrote: > On Sat, Aug 31, 2024 at 04:59:32PM +0200, Jacopo Mondi wrote: > > On Sat, Aug 31, 2024 at 04:17:56PM GMT, Laurent Pinchart wrote: > > > On Tue, Aug 27, 2024 at 09:40:16AM +0200, Jacopo Mondi wrote: > > > > The config parameters buffer is already validated in > > > > pisp_be_validate_config() at .buf_prepare() time. > > > > > > Unfortunately .buf_prepare() isn't the right place to handle the > > > validation. Userspace should not modify the contents of the buffer > > > before BUF_PREPARE and QBUF, but malicious (or just buggy) userspace > > > may. The validation should thus be moved to .buf_queue(). > > > > Probably right, but unrelated to this patch ? > > Yes, unrelated, but it should be fixed sooner than later as it's a > possible security issue. > > > > > However some of the same validations are also performed at > > > > pispbe_schedule() time. In particular the function checks that: > > > > > > > > 1) config.num_tiles is valid > > > > 2) At least one of the BAYER or RGB input is enabled > > > > > > > > The input validation is already performed in pisp_be_validate_config() > > > > and there is no need to repeat that at pispbe_schedule() time. > > > > > > Is that the same validation though ? The one in > > > pisp_be_validate_config() validates config->config.global, while the > > > validation in pispbe_schedule() validates job.hw_enables. The latter is > > > set from config->config.global in pispbe_xlate_addrs(), but is later > > > modified in the function. > > > > Ah yes, the ones validated at schedule() time are the ones in the job > > populated by pispbe_xlate_addrs(). > > > > However > > > > 1) config validation makes sure that in config->config.global enables > > at least one of BAYER_ENABLE_INPUT or RGB_ENABLE_INPUT is set > > > > 2) xlate_addrs() > > - resets both bayer_enable and rgb_enabl only if > > there's no main input buffer, which as replied in the previous > > email, shouldn't happen, otherwise prepare_job() fails before > > calling xlate_addrs() > > This is checked in pispbe_xlate_addrs by looking at the return value of > pispbe_get_planes_addr() for the main input. That function fails only > > if (!buf || !node->pisp_format) > return 0; > > buf should indeed not be NULL, as that is checked by > pispbe_prepare_job(). node->pisp_format should also never be NULL, as it > is initialized at probe time and should never be set to a NULL value > afterwards. That part should be fine. I think we should remove the > unneeded checks, they only contribute to making the code more > convoluted. I'd rather simplify and clarify checks in a single place to > give us enough certainty that further checks are not needed. Could you > submit follow-up patches for that ? > Indeed. Let's land this series in order not to pile too many things and work on moving the validation to buf_queue() and centralized and clean-up the sanity checks on top. > > - set bayer_enable = 0 if the BAYER_ENABLE_INPUT flag wasn't set in > > config->config.global (in which case rgb_enable is set because of > > the validation) > > > > - clear bit entries in rgb_enable but only for OUTPUTS not for > > input > > > > > > Which makes me think the validation in schedule() can be removed > > safely. > > > > A bit convoluted, yes, but possibily safe ? > > I think it's safe indeed. But it's definitely too convoluted :-) > > > > > The num_tiles validation can be moved to pisp_be_validate_config() as > > > > well. As num_tiles is a u32 it can'be be < 0, so change the sanity > > > > check accordingly. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > .../platform/raspberrypi/pisp_be/pisp_be.c | 25 ++++++------------- > > > > 1 file changed, 7 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > > index 8ba1b9f43ba1..73a5c88e25d0 100644 > > > > --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > > +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c > > > > @@ -588,24 +588,6 @@ static void pispbe_schedule(struct pispbe_dev *pispbe, bool clear_hw_busy) > > > > pispbe->hw_busy = true; > > > > spin_unlock_irqrestore(&pispbe->hw_lock, flags); > > > > > > > > - if (job.config->num_tiles <= 0 || > > > > - job.config->num_tiles > PISP_BACK_END_NUM_TILES || > > > > - !((job.hw_enables.bayer_enables | job.hw_enables.rgb_enables) & > > > > - PISP_BE_BAYER_ENABLE_INPUT)) { > > > > - /* > > > > - * Bad job. We can't let it proceed as it could lock up > > > > - * the hardware, or worse! > > > > - * > > > > - * For now, just force num_tiles to 0, which causes the > > > > - * H/W to do something bizarre but survivable. It > > > > - * increments (started,done) counters by more than 1, > > > > - * but we seem to survive... > > > > - */ > > > > - dev_dbg(pispbe->dev, "Bad job: invalid number of tiles: %u\n", > > > > - job.config->num_tiles); > > > > - job.config->num_tiles = 0; > > > > - } > > > > - > > > > pispbe_queue_job(pispbe, &job); > > > > > > > > return; > > > > @@ -703,6 +685,13 @@ static int pisp_be_validate_config(struct pispbe_dev *pispbe, > > > > return -EIO; > > > > } > > > > > > > > + if (config->num_tiles == 0 || > > > > + config->num_tiles > PISP_BACK_END_NUM_TILES) { > > > > + dev_dbg(dev, "%s: Invalid number of tiles: %d\n", __func__, > > > > + config->num_tiles); > > > > + return -EIO; > > > > > > Isn't -EINVAL a better error code ? > > > > > > > + } > > > > + > > > > /* Ensure output config strides and buffer sizes match the V4L2 formats. */ > > > > fmt = &pispbe->node[TDN_OUTPUT_NODE].format; > > > > if (bayer_enables & PISP_BE_BAYER_ENABLE_TDN_OUTPUT) { > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c index 8ba1b9f43ba1..73a5c88e25d0 100644 --- a/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c +++ b/drivers/media/platform/raspberrypi/pisp_be/pisp_be.c @@ -588,24 +588,6 @@ static void pispbe_schedule(struct pispbe_dev *pispbe, bool clear_hw_busy) pispbe->hw_busy = true; spin_unlock_irqrestore(&pispbe->hw_lock, flags); - if (job.config->num_tiles <= 0 || - job.config->num_tiles > PISP_BACK_END_NUM_TILES || - !((job.hw_enables.bayer_enables | job.hw_enables.rgb_enables) & - PISP_BE_BAYER_ENABLE_INPUT)) { - /* - * Bad job. We can't let it proceed as it could lock up - * the hardware, or worse! - * - * For now, just force num_tiles to 0, which causes the - * H/W to do something bizarre but survivable. It - * increments (started,done) counters by more than 1, - * but we seem to survive... - */ - dev_dbg(pispbe->dev, "Bad job: invalid number of tiles: %u\n", - job.config->num_tiles); - job.config->num_tiles = 0; - } - pispbe_queue_job(pispbe, &job); return; @@ -703,6 +685,13 @@ static int pisp_be_validate_config(struct pispbe_dev *pispbe, return -EIO; } + if (config->num_tiles == 0 || + config->num_tiles > PISP_BACK_END_NUM_TILES) { + dev_dbg(dev, "%s: Invalid number of tiles: %d\n", __func__, + config->num_tiles); + return -EIO; + } + /* Ensure output config strides and buffer sizes match the V4L2 formats. */ fmt = &pispbe->node[TDN_OUTPUT_NODE].format; if (bayer_enables & PISP_BE_BAYER_ENABLE_TDN_OUTPUT) {
The config parameters buffer is already validated in pisp_be_validate_config() at .buf_prepare() time. However some of the same validations are also performed at pispbe_schedule() time. In particular the function checks that: 1) config.num_tiles is valid 2) At least one of the BAYER or RGB input is enabled The input validation is already performed in pisp_be_validate_config() and there is no need to repeat that at pispbe_schedule() time. The num_tiles validation can be moved to pisp_be_validate_config() as well. As num_tiles is a u32 it can'be be < 0, so change the sanity check accordingly. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- .../platform/raspberrypi/pisp_be/pisp_be.c | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-)