| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3h5oa7ziojahyq4uxlpfdkqqqz2h2fakahjmtyv5un5yhxhat4@gborrcjbwme5>
Date: Tue, 19 Aug 2025 09:00:11 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Andy Walls <awalls@...metrocast.net>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil+cisco@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>, stable@...r.kernel.org,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] media: cx18: Fix invalid access to file *
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@...aro.org>
> > Closes: https://lore.kernel.org/all/aKL4OMWsESUdX8KQ@stanley.mountain/
> > Fixes: 7b9eb53e8591 ("media: cx18: Access v4l2_fh from file")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.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
Powered by blists - more mailing lists