Hi Laurent

On Tue, Aug 19, 2025 at 02:56:32AM +0300, Laurent Pinchart wrote:

On Mon, Aug 18, 2025 at 10:39:36PM +0200, Jacopo Mondi wrote:

...

Sice commit 7b9eb53e8591 ("media: cx18: Access v4l2_fh from file")

s/Sice/Since/

...

all ioctl handlers have been ported to operate on the file * first function argument.

The cx18 DVB layer calls cx18_init_on_first_open() when the driver needs to start streaming. This function calls the s_input(), s_std() and s_frequency() ioctl handlers directly, but being called from the driver context, it doesn't have a valid file * to pass them. This causes the ioctl handlers to deference an invalid pointer.

Fix this by wrapping the ioctl handlers implementation in helper functions which accepts a cx18 pointer as first argument and make the cx18_init_on_first_open() function call the helpers without going through the ioctl handlers.

It's the other way around, the ioctl handlers are not wrapper. I'd write

in facts

"wrapping the ioctl handlers implementation in helpers functions"

to me means wrapping the actual implementation in helpers

Fix this by moving the implementation of those ioctls to functions that

ah I should have used "moving" instead of "wrapping"

take a cx18 pointer instead of a file pointer, and turn the V4L2 ioctl handlers into wrappers that get the cx18 from the file. When calling from cx18_init_on_first_open(), pass the cx18 pointer directly. This allows removing the fake fh in cx18_init_on_first_open().

ok, if it's -that- different... thankfully we nowadays have b4 that makes sending new version easier

...

The bug has been reported by Smatch:

--> 1223 cx18_s_input(NULL, &fh, video_input); The patch adds a new dereference of "file" but some of the callers pass a NULL pointer.

Reported-by: Dan Carpenter dan.carpenter@linaro.org Closes: https://lore.kernel.org/all/aKL4OMWsESUdX8KQ@stanley.mountain/ Fixes: 7b9eb53e8591 ("media: cx18: Access v4l2_fh from file") Cc: stable@vger.kernel.org Signed-off-by: Jacopo Mondi jacopo.mondi@ideasonboard.com

Reviewed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

...

---

drivers/media/pci/cx18/cx18-driver.c | 9 +++------ drivers/media/pci/cx18/cx18-ioctl.c | 30 +++++++++++++++++++----------- drivers/media/pci/cx18/cx18-ioctl.h | 8 +++++--- 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/media/pci/cx18/cx18-driver.c b/drivers/media/pci/cx18/cx18-driver.c index 743fcc9613744bfc1edeffc51e908fe88520405a..cd84dfcefcf971a7adb9aac2bafb9089dbe0f33f 100644 --- a/drivers/media/pci/cx18/cx18-driver.c +++ b/drivers/media/pci/cx18/cx18-driver.c @@ -1136,11 +1136,8 @@ int cx18_init_on_first_open(struct cx18 *cx) int video_input; int fw_retry_count = 3; struct v4l2_frequency vf;

• struct cx18_open_id fh; v4l2_std_id std;

• fh.cx = cx;

• if (test_bit(CX18_F_I_FAILED, &cx->i_flags)) return -ENXIO;

@@ -1220,14 +1217,14 @@ int cx18_init_on_first_open(struct cx18 *cx)

video_input = cx->active_input; cx->active_input++; /* Force update of input */

• cx18_s_input(NULL, &fh, video_input);

• cx18_do_s_input(cx, video_input);

  /* Let the VIDIOC_S_STD ioctl do all the work, keeps the code in one place. */ cx->std++; /* Force full standard initialization */ std = (cx->tuner_std == V4L2_STD_ALL) ? V4L2_STD_NTSC_M : cx->tuner_std;

• cx18_s_std(NULL, &fh, std);
• cx18_s_frequency(NULL, &fh, &vf);

• cx18_do_s_std(cx, std);
• cx18_do_s_frequency(cx, &vf); return 0;

}

diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c index bf16d36448f888d9326b5f4a8f9c8f0e13d0c3a1..6e869c43cbd520feb720a71d8eb2dd60c05b0ae9 100644 --- a/drivers/media/pci/cx18/cx18-ioctl.c +++ b/drivers/media/pci/cx18/cx18-ioctl.c @@ -521,10 +521,8 @@ static int cx18_g_input(struct file *file, void *fh, unsigned int *i) return 0; }

-int cx18_s_input(struct file *file, void *fh, unsigned int inp) +int cx18_do_s_input(struct cx18 *cx, unsigned int inp) {

• struct cx18_open_id *id = file2id(file);
• struct cx18 *cx = id->cx; v4l2_std_id std = V4L2_STD_ALL; const struct cx18_card_video_input *card_input = cx->card->video_inputs + inp;

@@ -558,6 +556,11 @@ int cx18_s_input(struct file *file, void *fh, unsigned int inp) return 0; }

+static int cx18_s_input(struct file *file, void *fh, unsigned int inp) +{

• return cx18_do_s_input(file2id(file)->cx, inp);

+}

static int cx18_g_frequency(struct file *file, void *fh, struct v4l2_frequency *vf) { @@ -570,11 +573,8 @@ static int cx18_g_frequency(struct file *file, void *fh, return 0; }

-int cx18_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *vf) +int cx18_do_s_frequency(struct cx18 *cx, const struct v4l2_frequency *vf) {

• struct cx18_open_id *id = file2id(file);
• struct cx18 *cx = id->cx;
• if (vf->tuner != 0) return -EINVAL;

@@ -585,6 +585,12 @@ int cx18_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *v return 0; }

+static int cx18_s_frequency(struct file *file, void *fh,

• 	    const struct v4l2_frequency *vf)
  

+{

• return cx18_do_s_frequency(file2id(file)->cx, vf);

+}

static int cx18_g_std(struct file *file, void *fh, v4l2_std_id *std) { struct cx18 *cx = file2id(file)->cx; @@ -593,11 +599,8 @@ static int cx18_g_std(struct file *file, void *fh, v4l2_std_id *std) return 0; }

-int cx18_s_std(struct file *file, void *fh, v4l2_std_id std) +int cx18_do_s_std(struct cx18 *cx, v4l2_std_id std) {

• struct cx18_open_id *id = file2id(file);
• struct cx18 *cx = id->cx;
• if ((std & V4L2_STD_ALL) == 0) return -EINVAL;

@@ -642,6 +645,11 @@ int cx18_s_std(struct file *file, void *fh, v4l2_std_id std) return 0; }

+static int cx18_s_std(struct file *file, void *fh, v4l2_std_id std) +{

• return cx18_do_s_std(file2id(file)->cx, std);

+}

static int cx18_s_tuner(struct file *file, void *fh, const struct v4l2_tuner *vt) { struct cx18_open_id *id = file2id(file); diff --git a/drivers/media/pci/cx18/cx18-ioctl.h b/drivers/media/pci/cx18/cx18-ioctl.h index 221e2400fb3e2d817eaff7515fa89eb94f2d7f8a..7a42ac99312ab6502e1abe4f3d5c88c9c7f144f3 100644 --- a/drivers/media/pci/cx18/cx18-ioctl.h +++ b/drivers/media/pci/cx18/cx18-ioctl.h @@ -12,6 +12,8 @@ u16 cx18_service2vbi(int type); void cx18_expand_service_set(struct v4l2_sliced_vbi_format *fmt, int is_pal); u16 cx18_get_service_set(struct v4l2_sliced_vbi_format *fmt); void cx18_set_funcs(struct video_device *vdev); -int cx18_s_std(struct file *file, void *fh, v4l2_std_id std); -int cx18_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *vf); -int cx18_s_input(struct file *file, void *fh, unsigned int inp);

+struct cx18; +int cx18_do_s_std(struct cx18 *cx, v4l2_std_id std); +int cx18_do_s_frequency(struct cx18 *cx, const struct v4l2_frequency *vf); +int cx18_do_s_input(struct cx18 *cx, unsigned int inp);

-- Regards,

Laurent Pinchart

Hi Jacopo,

Thank you for the patch.

On Mon, Aug 18, 2025 at 10:39:37PM +0200, Jacopo Mondi wrote:

Since commit 9ba9d11544f9 ("media: ivtv: Access v4l2_fh from file") all ioctl handlers have been ported to operate on the file * first function argument.

The ivtv DVB layer calls ivtv_init_on_first_open() when the driver needs to start streaming. This function calls the s_input() and s_frequency() ioctl handlers directly, but being called from the driver context, it doesn't have a valid file * to pass them. This causes the ioctl handlers to deference an invalid pointer.

Fix this by wrapping the ioctl handlers implementation in helper functions.

You may want to reword this in a similar way as proposed in 1/2.

The ivtv_do_s_input() helper accepts a struct ivtv * as first argument, which is easily accessible from the DVB layer as well as from the file * argument of the ioctl handler.

The ivtv_s_frequency() takes an ivtv_stream * instead. The stream * can safely be accessed from the DVB layer which hard-codes it to the IVTV_ENC_STREAM_TYPE_MPG stream type, as well as from the ioctl handler a valid stream type is associated to each open file handle depending on which video device node has been opened in the ivtv_open() file operation.

The bug has been reported by Smatch.

Reported-by: Dan Carpenter dan.carpenter@linaro.org Closes: https://lore.kernel.org/all/aKL4OMWsESUdX8KQ@stanley.mountain/ Fixes: 9ba9d11544f9 ("media: ivtv: Access v4l2_fh from file") Cc: stable@vger.kernel.org Signed-off-by: Jacopo Mondi jacopo.mondi@ideasonboard.com

---

drivers/media/pci/ivtv/ivtv-driver.c | 11 ++++------- drivers/media/pci/ivtv/ivtv-ioctl.c | 22 +++++++++++++++++----- drivers/media/pci/ivtv/ivtv-ioctl.h | 6 ++++-- 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtv-driver.c b/drivers/media/pci/ivtv/ivtv-driver.c index ecc20cd89926fe2ce4e472526a6b5fc0857615dd..7e2fb98cfccf02f701ceb4484dd1d330dd1dc867 100644 --- a/drivers/media/pci/ivtv/ivtv-driver.c +++ b/drivers/media/pci/ivtv/ivtv-driver.c @@ -1260,15 +1260,12 @@ static int ivtv_probe(struct pci_dev *pdev, const struct pci_device_id *pci_id) int ivtv_init_on_first_open(struct ivtv *itv) {

• struct v4l2_frequency vf; /* Needed to call ioctls later */

I'd drop the comment.

• struct ivtv_open_id fh;

• struct ivtv_stream *s = &itv->streams[IVTV_ENC_STREAM_TYPE_MPG];
• struct v4l2_frequency vf; int fw_retry_count = 3; int video_input;

• fh.itv = itv;
• fh.type = IVTV_ENC_STREAM_TYPE_MPG;
• if (test_bit(IVTV_F_I_FAILED, &itv->i_flags)) return -ENXIO;

@@ -1310,13 +1307,13 @@ int ivtv_init_on_first_open(struct ivtv *itv) video_input = itv->active_input; itv->active_input++; /* Force update of input */

• ivtv_s_input(NULL, &fh, video_input);

• ivtv_do_s_input(itv, video_input);

/* Let the VIDIOC_S_STD ioctl do all the work, keeps the code in one place. */ itv->std++; /* Force full standard initialization */ itv->std_out = itv->std;

• ivtv_s_frequency(NULL, &fh, &vf);

• ivtv_do_s_frequency(s, &vf);

ivtv_do_s_frequency(&itv->streams[IVTV_ENC_STREAM_TYPE_MPG], &vf);

would work too. Up to you.

if (itv->card->v4l2_capabilities & V4L2_CAP_VIDEO_OUTPUT) { /* Turn on the TV-out: ivtv_init_mpeg_decoder() initializes diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.c b/drivers/media/pci/ivtv/ivtv-ioctl.c index 8077a71d4850ec773caa20c3fca08f92f3117d69..dfbc842b22453868a2075935a81db7ae313ee46c 100644 --- a/drivers/media/pci/ivtv/ivtv-ioctl.c +++ b/drivers/media/pci/ivtv/ivtv-ioctl.c @@ -974,9 +974,8 @@ static int ivtv_g_input(struct file *file, void *fh, unsigned int *i) return 0; } -int ivtv_s_input(struct file *file, void *fh, unsigned int inp) +int ivtv_do_s_input(struct ivtv *itv, unsigned int inp) {

• struct ivtv *itv = file2id(file)->itv; v4l2_std_id std; int i;

@@ -1017,6 +1016,11 @@ int ivtv_s_input(struct file *file, void *fh, unsigned int inp) return 0; } +static int ivtv_s_input(struct file *file, void *fh, unsigned int inp) +{

• return ivtv_do_s_input(file2id(file)->itv, inp);

+}

static int ivtv_g_output(struct file *file, void *fh, unsigned int *i) { struct ivtv *itv = file2id(file)->itv; @@ -1065,10 +1069,9 @@ static int ivtv_g_frequency(struct file *file, void *fh, struct v4l2_frequency * return 0; } -int ivtv_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *vf) +int ivtv_do_s_frequency(struct ivtv_stream *s, const struct v4l2_frequency *vf) {

• struct ivtv *itv = file2id(file)->itv;
• struct ivtv_stream *s = &itv->streams[file2id(file)->type];

• struct ivtv *itv = s->itv;

if (s->vdev.vfl_dir) return -ENOTTY; @@ -1082,6 +1085,15 @@ int ivtv_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *v return 0; } +static int ivtv_s_frequency(struct file *file, void *fh,

• 	    const struct v4l2_frequency *vf)
  

+{

• struct ivtv_open_id *id = file2id(file);
• struct ivtv *itv = id->itv;
• return ivtv_do_s_frequency(&itv->streams[id->type], vf);

+}

static int ivtv_g_std(struct file *file, void *fh, v4l2_std_id *std) { struct ivtv *itv = file2id(file)->itv; diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.h b/drivers/media/pci/ivtv/ivtv-ioctl.h index 42c2516379fcbbd0640820ab0e3abe9bf00b57ea..dd713a6b095e5ebca45a234dd6c9a90df0928596 100644 --- a/drivers/media/pci/ivtv/ivtv-ioctl.h +++ b/drivers/media/pci/ivtv/ivtv-ioctl.h @@ -17,7 +17,9 @@ int ivtv_set_speed(struct ivtv *itv, int speed); void ivtv_set_funcs(struct video_device *vdev); void ivtv_s_std_enc(struct ivtv *itv, v4l2_std_id std); void ivtv_s_std_dec(struct ivtv *itv, v4l2_std_id std); -int ivtv_s_frequency(struct file *file, void *fh, const struct v4l2_frequency *vf); -int ivtv_s_input(struct file *file, void *fh, unsigned int inp);

+struct ivtv;

I'd drop this, as the structure is already used above.

Reviewed-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

+int ivtv_do_s_frequency(struct ivtv_stream *s, const struct v4l2_frequency *vf); +int ivtv_do_s_input(struct ivtv *itv, unsigned int inp); #endif