| Message ID | 20190829110032.4187-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vimc: upon streaming, check that the pipeline starts with a source entity | expand |
Hello Dafna, Thanks for your patch! On 8/29/19 8:00 AM, Dafna Hirschfeld wrote: > Userspace can disable links and create pipelines that > do not start with a source entity. Trying to stream > from such a pipeline should fail with -EPIPE > currently this is not handled and cause kernel crash. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > Hi, > These are the commands to reproduce the crash: > media-ctl -d0 -l "5:1->21:0[0]" -v > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 > v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2 > > drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------ > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c > index 048d770e498b..bfc7921c7a8b 100644 > --- a/drivers/media/platform/vimc/vimc-streamer.c > +++ b/drivers/media/platform/vimc/vimc-streamer.c > @@ -13,6 +13,19 @@ > > #include "vimc-streamer.h" > > +/** > + * Check if the entity has only source pads > + */ You should only use /** when you are writing kernel-doc comments. You may either replace /** by /*, or add a function documentation[1]. Thanks, André [1] https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation > +static bool vimc_is_source(struct media_entity *ent) > +{ > + int i; > + > + for (i = 0; i < ent->num_pads; i++) > + if (ent->pads[i].flags & MEDIA_PAD_FL_SINK) > + return false; > + return true; > +} > + > /** > * vimc_get_source_entity - get the entity connected with the first sink pad > * > @@ -83,14 +96,12 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > struct media_entity *entity; > struct video_device *vdev; > struct v4l2_subdev *sd; > - int ret = 0; > + int ret = -EINVAL; > > stream->pipe_size = 0; > while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > - if (!ved) { > - vimc_streamer_pipeline_terminate(stream); > - return -EINVAL; > - } > + if (!ved) > + break; > stream->ved_pipeline[stream->pipe_size++] = ved; > > if (is_media_entity_v4l2_subdev(ved->ent)) { > @@ -99,15 +110,22 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > if (ret && ret != -ENOIOCTLCMD) { > pr_err("subdev_call error %s\n", > ved->ent->name); > - vimc_streamer_pipeline_terminate(stream); > - return ret; > + break; > } > } > > entity = vimc_get_source_entity(ved->ent); > - /* Check if the end of the pipeline was reached*/ > - if (!entity) > + /* Check if the end of the pipeline was reached */ > + if (!entity) { > + /* the first entity of the pipe should be source only */ > + if (!vimc_is_source(ved->ent)) { > + pr_err("first entity in the pipe '%s' is not a source\n", > + ved->ent->name); > + ret = -EPIPE; > + break; > + } > return 0; > + } > > /* Get the next device in the pipeline */ > if (is_media_entity_v4l2_subdev(entity)) { > @@ -120,9 +138,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > ved = video_get_drvdata(vdev); > } > } > - > vimc_streamer_pipeline_terminate(stream); > - return -EINVAL; > + return ret; > } > > /**
Hi Dafna, Thanks for the patch. Just small comments below. On 8/29/19 8:00 AM, Dafna Hirschfeld wrote: > Userspace can disable links and create pipelines that > do not start with a source entity. Trying to stream > from such a pipeline should fail with -EPIPE > currently this is not handled and cause kernel crash. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > Hi, > These are the commands to reproduce the crash: > media-ctl -d0 -l "5:1->21:0[0]" -v > v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 > v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2 > > drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------ > 1 file changed, 28 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c > index 048d770e498b..bfc7921c7a8b 100644 > --- a/drivers/media/platform/vimc/vimc-streamer.c > +++ b/drivers/media/platform/vimc/vimc-streamer.c > @@ -13,6 +13,19 @@ > > #include "vimc-streamer.h" > > +/** > + * Check if the entity has only source pads > + */ please add docs like the other functions in this file. > +static bool vimc_is_source(struct media_entity *ent) > +{ > + int i; unsigned Regards, Helen > + > + for (i = 0; i < ent->num_pads; i++) > + if (ent->pads[i].flags & MEDIA_PAD_FL_SINK) > + return false; > + return true; > +} > + > /** > * vimc_get_source_entity - get the entity connected with the first sink pad > * > @@ -83,14 +96,12 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > struct media_entity *entity; > struct video_device *vdev; > struct v4l2_subdev *sd; > - int ret = 0; > + int ret = -EINVAL; > > stream->pipe_size = 0; > while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { > - if (!ved) { > - vimc_streamer_pipeline_terminate(stream); > - return -EINVAL; > - } > + if (!ved) > + break; > stream->ved_pipeline[stream->pipe_size++] = ved; > > if (is_media_entity_v4l2_subdev(ved->ent)) { > @@ -99,15 +110,22 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > if (ret && ret != -ENOIOCTLCMD) { > pr_err("subdev_call error %s\n", > ved->ent->name); > - vimc_streamer_pipeline_terminate(stream); > - return ret; > + break; > } > } > > entity = vimc_get_source_entity(ved->ent); > - /* Check if the end of the pipeline was reached*/ > - if (!entity) > + /* Check if the end of the pipeline was reached */ > + if (!entity) { > + /* the first entity of the pipe should be source only */ > + if (!vimc_is_source(ved->ent)) { > + pr_err("first entity in the pipe '%s' is not a source\n", > + ved->ent->name); > + ret = -EPIPE; > + break; > + } > return 0; > + } > > /* Get the next device in the pipeline */ > if (is_media_entity_v4l2_subdev(entity)) { > @@ -120,9 +138,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, > ved = video_get_drvdata(vdev); > } > } > - > vimc_streamer_pipeline_terminate(stream); > - return -EINVAL; > + return ret; > } > > /** >
diff --git a/drivers/media/platform/vimc/vimc-streamer.c b/drivers/media/platform/vimc/vimc-streamer.c index 048d770e498b..bfc7921c7a8b 100644 --- a/drivers/media/platform/vimc/vimc-streamer.c +++ b/drivers/media/platform/vimc/vimc-streamer.c @@ -13,6 +13,19 @@ #include "vimc-streamer.h" +/** + * Check if the entity has only source pads + */ +static bool vimc_is_source(struct media_entity *ent) +{ + int i; + + for (i = 0; i < ent->num_pads; i++) + if (ent->pads[i].flags & MEDIA_PAD_FL_SINK) + return false; + return true; +} + /** * vimc_get_source_entity - get the entity connected with the first sink pad * @@ -83,14 +96,12 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, struct media_entity *entity; struct video_device *vdev; struct v4l2_subdev *sd; - int ret = 0; + int ret = -EINVAL; stream->pipe_size = 0; while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) { - if (!ved) { - vimc_streamer_pipeline_terminate(stream); - return -EINVAL; - } + if (!ved) + break; stream->ved_pipeline[stream->pipe_size++] = ved; if (is_media_entity_v4l2_subdev(ved->ent)) { @@ -99,15 +110,22 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, if (ret && ret != -ENOIOCTLCMD) { pr_err("subdev_call error %s\n", ved->ent->name); - vimc_streamer_pipeline_terminate(stream); - return ret; + break; } } entity = vimc_get_source_entity(ved->ent); - /* Check if the end of the pipeline was reached*/ - if (!entity) + /* Check if the end of the pipeline was reached */ + if (!entity) { + /* the first entity of the pipe should be source only */ + if (!vimc_is_source(ved->ent)) { + pr_err("first entity in the pipe '%s' is not a source\n", + ved->ent->name); + ret = -EPIPE; + break; + } return 0; + } /* Get the next device in the pipeline */ if (is_media_entity_v4l2_subdev(entity)) { @@ -120,9 +138,8 @@ static int vimc_streamer_pipeline_init(struct vimc_stream *stream, ved = video_get_drvdata(vdev); } } - vimc_streamer_pipeline_terminate(stream); - return -EINVAL; + return ret; } /**
Userspace can disable links and create pipelines that do not start with a source entity. Trying to stream from such a pipeline should fail with -EPIPE currently this is not handled and cause kernel crash. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- Hi, These are the commands to reproduce the crash: media-ctl -d0 -l "5:1->21:0[0]" -v v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440 v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2 drivers/media/platform/vimc/vimc-streamer.c | 39 +++++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-)