On Wed, Jun 12, 2024 at 03:28:05PM +0100, Daniel Scally wrote:
> Hi Jacopo. As mentioned in the last patch, I think that this could be
> squashed into 5/8, but a couple of comments below
I think it should be moved earlier, yes, probably even before the
introduction of extended parameters.
> On 05/06/2024 17:54, Jacopo Mondi wrote:
> > With the introduction of the extensible parameters format support in the
> > rkisp1-param.c module, the RkISP1 driver now configures the ISP blocks
> > by parsing the content of a data buffer of variable size provided by
> > userspace through the V4L2 meta-output interface using the MMAP memory
> > handling mode.
> >
> > As the parameters buffer is mapped in the userspace process memory,
> > applications have access to the buffer content while the driver
> > parses it.
> >
> > To prevent potential issues during the parameters buffer parsing and
> > processing in the driver, implement three vb2_ops to
> >
> > 1) allocate a scratch buffer in the driver private buffer structure
> > 2) validate the buffer content at VIDIOC_QBUF time
> > 3) copy the content of the user provided configuration parameters
> > in the driver-private scratch buffer
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 154 ++++++++++++++----
> > 1 file changed, 124 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 4adaf084ce6e..003239e14511 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -5,6 +5,8 @@
> > * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > */
> >
> > +#include <linux/string.h>
> > +
> > #include <media/v4l2-common.h>
> > #include <media/v4l2-event.h>
> > #include <media/v4l2-ioctl.h>
> > @@ -1943,17 +1945,14 @@ static const struct rkisp1_ext_params_handler {
> > };
> >
> > static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > - struct rkisp1_ext_params_cfg *cfg,
> > + struct rkisp1_params_buffer *buffer,
> > u32 block_group_mask)
> > {
> > + struct rkisp1_ext_params_cfg *cfg = buffer->cfg;
Maybe do this in the callers to avoid changing the prototype of this
function and the other functions below.
> > size_t block_offset = 0;
> >
> > - if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > - dev_dbg(params->rkisp1->dev,
> > - "Invalid parameters buffer size %llu\n",
> > - cfg->total_size);
> > - return -EINVAL;
> > - }
> > + if (WARN_ON(!cfg))
> > + return -ENOMEM;
> >
> > /* Walk the list of parameter blocks and process them. */
> > while (block_offset < cfg->total_size) {
> > @@ -1965,25 +1964,13 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > block_offset += block->size;
> >
> > /*
> > - * Validate the block id and make sure the block group is in
> > - * the list of groups to configure.
> > + * Make sure the block group is in the list of groups to
> > + * configure.
> > */
> > - if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
> > - dev_dbg(params->rkisp1->dev,
> > - "Invalid parameters block type\n");
> > - return -EINVAL;
> > - }
> > -
> > block_handler = &rkisp1_ext_params_handlers[block->type];
> > if (!(block_handler->group & block_group_mask))
> > continue;
> >
> > - if (block->size != block_handler->size) {
> > - dev_dbg(params->rkisp1->dev,
> > - "Invalid parameters block size\n");
> > - return -EINVAL;
> > - }
> > -
> > block_handler->handler(params, block);
> > }
> >
> > @@ -1991,9 +1978,9 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > }
> >
> > static int rkisp1_ext_params_config(struct rkisp1_params *params,
> > - struct rkisp1_ext_params_cfg *cfg)
> > + struct rkisp1_params_buffer *buffer)
> > {
> > - return __rkisp1_ext_params_config(params, cfg,
> > + return __rkisp1_ext_params_config(params, buffer,
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC |
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > @@ -2001,17 +1988,17 @@ static int rkisp1_ext_params_config(struct rkisp1_params *params,
> >
> > static int
> > rkisp1_ext_params_other_meas_config(struct rkisp1_params *params,
> > - struct rkisp1_ext_params_cfg *cfg)
> > + struct rkisp1_params_buffer *buffer)
> > {
> > - return __rkisp1_ext_params_config(params, cfg,
> > + return __rkisp1_ext_params_config(params, buffer,
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > }
> >
> > static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params,
> > - struct rkisp1_ext_params_cfg *cfg)
> > + struct rkisp1_params_buffer *buffer)
> > {
> > - return __rkisp1_ext_params_config(params, cfg,
> > + return __rkisp1_ext_params_config(params, buffer,
> > RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
> > }
> >
> > @@ -2057,7 +2044,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > rkisp1_isp_isr_lsc_config(params, cfg);
> > rkisp1_isp_isr_meas_config(params, cfg);
> > } else {
> > - ret = rkisp1_ext_params_config(params, cfg);
> > + ret = rkisp1_ext_params_config(params, buf);
> > }
> >
> > if (ret)
> > @@ -2168,7 +2155,7 @@ int rkisp1_params_pre_configure(struct rkisp1_params *params,
> > rkisp1_isp_isr_other_config(params, cfg);
> > rkisp1_isp_isr_meas_config(params, cfg);
> > } else {
> > - ret = rkisp1_ext_params_other_meas_config(params, cfg);
> > + ret = rkisp1_ext_params_other_meas_config(params, buf);
> > }
> >
> > if (ret) {
> > @@ -2215,7 +2202,7 @@ int rkisp1_params_post_configure(struct rkisp1_params *params)
> > if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> > rkisp1_isp_isr_lsc_config(params, cfg);
> > else
> > - ret = rkisp1_ext_params_lsc_config(params, cfg);
> > + ret = rkisp1_ext_params_lsc_config(params, buf);
> >
> > if (ret)
> > goto complete_and_unlock;
> > @@ -2407,6 +2394,110 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
> > return 0;
> > }
> >
> > +static int rkisp1_params_vb2_buf_init(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct rkisp1_params_buffer *params_buf =
> > + container_of(vbuf, struct rkisp1_params_buffer, vb);
A to_rkisp1_params_buffer() inline function in the previous patch would
be nice.
> > + struct rkisp1_params *params = vb->vb2_queue->drv_priv;
> > + size_t buf_size = params->metafmt.buffersize;
> > +
> > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > + params_buf->cfg = NULL;
> > + return 0;
> > + }
The problem is not restricted to the extensible format, how about doing
the same for the legacy format ?
> > +
> > + params_buf->cfg = kvmalloc(buf_size, GFP_KERNEL);
> > + if (IS_ERR_OR_NULL(params_buf->cfg))
Can kvmalloc() return an error pointer ?
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static void rkisp1_params_vb2_buf_cleanup(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct rkisp1_params_buffer *params_buf =
> > + container_of(vbuf, struct rkisp1_params_buffer, vb);
> > +
> > + kvfree(params_buf->cfg);
params_buf->cfg = NULL;
to be safe.
> > +}
> > +
> > +static int rkisp1_params_validate_ext_params(struct rkisp1_params *params,
> > + struct rkisp1_ext_params_cfg *cfg)
> > +{
> > + size_t block_offset = 0;
> > +
> > + if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters buffer size %llu\n",
> > + cfg->total_size);
> > + return -EINVAL;
> > + }
> > +
> > + /* Walk the list of parameter blocks and validate them. */
> > + while (block_offset < cfg->total_size) {
> > + const struct rkisp1_ext_params_handler *hdlr;
> > + struct rkisp1_ext_params_block_header *block;
> > +
> > + block = (struct rkisp1_ext_params_block_header *)
> > + &cfg->data[block_offset];
> > + block_offset += block->size;
Move this line to the end of the loop to avoid block_offset ever
pointing beyond the end of the buffer.
> > +
> > + if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters block type\n");
> > + return -EINVAL;
> > + }
> > +
> > + hdlr = &rkisp1_ext_params_handlers[block->type];
> > + if (hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS &&
> > + hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC &&
> > + hdlr->group != RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters block group\n");
> > + return -EINVAL;
> > + }
>
> I think this check can probably be dropped; those values are from the
> kernel driver rather than userspace inputs.
>
> > +
> > + if (block->size != hdlr->size) {
> > + dev_dbg(params->rkisp1->dev,
> > + "Invalid parameters block size\n");
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int rkisp1_params_vb2_buf_out_validate(struct vb2_buffer *vb)
> > +{
> > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > + struct rkisp1_params_buffer *params_buf =
> > + container_of(vbuf, struct rkisp1_params_buffer, vb);
> > + struct vb2_queue *vq = vb->vb2_queue;
> > + struct rkisp1_params *params = vq->drv_priv;
> > + struct rkisp1_ext_params_cfg *cfg =
> > + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0);
> > + int ret;
> > +
> > + /* Fixed parameters format doesn't require validation. */
> > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> > + return 0;
You need to add a check to rkisp1_params_s_fmt_meta_out() to reject
format changes once the queue is busy, or you'll have bad surprises.
> > +
> > + ret = rkisp1_params_validate_ext_params(params, cfg);
> > + if (ret)
> > + return ret;
> > +
> > + /*
> > + * If the parameters buffer is valid, copy it to the internal scratch
> > + * buffer to avoid userspace modifying the buffer content while
> > + * the driver processes it.
> > + */
You need to swap the validation and memcpy(), otherwise userspace could
modify it after you have validated the contents and before the copy.
> > + memcpy(params_buf->cfg, cfg, sizeof(*cfg));
>
> I think that this part is something that we probably ought to handle
> in vb2 core if we can, since the problem it's fixing isn't specific to
> the extensible parameters format or even the rkisp1 itself (unless I'm
> missing something). That doesn't have to block this set though, we can
> change this over to a vb2-core implementation when that's done.
I like the idea. I think we can experiment with it in rkisp1, and then
move it to vb2 when adding a second implementation (likely C55 ?).
> > +
> > + return 0;
> > +}
> > +
> > static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
> > {
> > struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > @@ -2455,6 +2546,9 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
> >
> > static const struct vb2_ops rkisp1_params_vb2_ops = {
> > .queue_setup = rkisp1_params_vb2_queue_setup,
> > + .buf_init = rkisp1_params_vb2_buf_init,
> > + .buf_cleanup = rkisp1_params_vb2_buf_cleanup,
> > + .buf_out_validate = rkisp1_params_vb2_buf_out_validate,
> > .wait_prepare = vb2_ops_wait_prepare,
> > .wait_finish = vb2_ops_wait_finish,
> > .buf_queue = rkisp1_params_vb2_buf_queue,
--
Regards,
Laurent Pinchart
