linux-kernel - Re: [PATCH] v4l: subdev: initialize sd->internal_ops in v4l2_subdev

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201104011705.27742.hverkuil@xs4all.nl>
Date:	Fri, 1 Apr 2011 17:05:27 +0200
From:	Hans Verkuil <hverkuil@...all.nl>
To:	Herton Ronaldo Krzesinski <herton.krzesinski@...onical.com>
Cc:	Mauro Carvalho Chehab <mchehab@...radead.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] v4l: subdev: initialize sd->internal_ops in v4l2_subdev_init

On Friday, April 01, 2011 16:24:17 Herton Ronaldo Krzesinski wrote:
> Many v4l drivers currently don't initialize their struct v4l2_subdev
> with zeros, so since the addition of internal_ops in commit 45f6f84, we
> are at risk of random oopses when code in v4l2_device_register_subdev
> tries to dereference sd->internal_ops->*, as can be shown by the report
> at http://bugs.launchpad.net/bugs/745213
> 
> So make sure internal_ops is cleared in v4l2_subdev_init.

NACK: we need to replace those kmalloc's with kzalloc. This patch will just
fix the internal_ops problem, but sd->entity isn't zeroed. It's going to be
a neverending problem unless we fix those kmalloc's. It is my fault, I guess:
I should always have required the use of kzalloc.

I grepped for this and the number of kmalloc's is quite small:

drivers/media/video/tda9840.c
drivers/media/video/upd64031a.c
drivers/media/video/m52790.c
drivers/media/video/tea6415c.c
drivers/media/video/tea6420.c
drivers/media/video/upd64083.c
drivers/media/radio/saa7706h.c
drivers/media/radio/tef6862.c

If you fix those kmalloc's, then I'll ack those changes. There is just one
relevant kmalloc in each file.

The reason this wasn't noticed before is that all these devices are all
pretty rare. All the more common device drivers use kzalloc. I am really
surprised to hear of mxb boards still in use! Just for the record: I have
one myself for testing, although I clearly never realized that I should
test that particular change with that board...

Thanks for doing such a great job of tracking this down!

Regards,

	Hans

> 
> BugLink: http://bugs.launchpad.net/bugs/745213
> Cc: <stable@...nel.org> # .38.x
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@...onical.com>
> ---
>  drivers/media/video/v4l2-subdev.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
> index 0b80644..0f70c74 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -324,6 +324,7 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>  	sd->grp_id = 0;
>  	sd->dev_priv = NULL;
>  	sd->host_priv = NULL;
> +	sd->internal_ops = NULL;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	sd->entity.name = sd->name;
>  	sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/