linux-kernel - RE: [PATCH] video_device: don't free_irq() an element past array vpif

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: <A69FA2915331DC488A831521EAE36FE40169C5C9B5@dlee06.ent.ti.com>
Date:	Thu, 18 Feb 2010 10:31:49 -0600
From:	"Karicheri, Muralidharan" <m-karicheri2@...com>
To:	Roel Kluin <roel.kluin@...il.com>,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] video_device: don't free_irq() an element past array
 vpif_obj.dev[] and fix test

Roel,

Thanks for the patch. 

>In vpif_get_std_info(): std_info doesn't need the NULL test, it was already
>dereferenced anyway. If std_info->stdid is 0 we could early return -1.
>
>In vpif_probe(): local variable q was only assigned. If we error out with
>either last two goto's then j equals VPIF_DISPLAY_MAX_DEVICES. So after the
>probe_out: label, k also reaches VPIF_DISPLAY_MAX_DEVICES after the loop.
>In
>the first iteration in the loop after vpif_int_err: a free_irq() can occur
>of an element &vpif_obj.dev[VPIF_DISPLAY_MAX_DEVICES]->channel_id which is
>outside vpif_obj.dev[] array boundaries.
>
>Signed-off-by: Roel Kluin <roel.kluin@...il.com>
>---
>Or am I mistaken?
>
>diff --git a/drivers/media/video/davinci/vpif_display.c
>b/drivers/media/video/davinci/vpif_display.c
>index dfddef7..8f6605d 100644
>--- a/drivers/media/video/davinci/vpif_display.c
>+++ b/drivers/media/video/davinci/vpif_display.c
>@@ -383,7 +383,7 @@ static int vpif_get_std_info(struct channel_obj *ch)
> 	int index;
>
> 	std_info->stdid = vid_ch->stdid;
>-	if (!std_info)
>+	if (!std_info->stdid)
> 		return -1;
>
This is a NACK. We shouldn't check for stdid since the function is supposed
to update std_info. So just remove

if (!std_info)
	return -1;

I am okay with the below change. So please re-submit the patch with the 
above change and my ACK.

Thanks

Murali

> 	for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
>@@ -1423,7 +1423,7 @@ static __init int vpif_probe(struct platform_device
>*pdev)
> {
> 	struct vpif_subdev_info *subdevdata;
> 	struct vpif_display_config *config;
>-	int i, j = 0, k, q, m, err = 0;
>+	int i, j = 0, k, m, err = 0;
> 	struct i2c_adapter *i2c_adap;
> 	struct common_obj *common;
> 	struct channel_obj *ch;
>@@ -1573,10 +1573,12 @@ probe_out:
> 		video_device_release(ch->video_dev);
> 		ch->video_dev = NULL;
> 	}
>+	if (k == VPIF_DISPLAY_MAX_DEVICES)
>+		k = VPIF_DISPLAY_MAX_DEVICES - 1;
> vpif_int_err:
> 	v4l2_device_unregister(&vpif_obj.v4l2_dev);
> 	vpif_err("VPIF IRQ request failed\n");
>-	for (q = k; k >= 0; k--) {
>+	for (; k >= 0; k--) {
> 		for (m = i; m >= res->start; m--)
> 			free_irq(m, (void *)(&vpif_obj.dev[k]->channel_id));
> 		res = platform_get_resource(pdev, IORESOURCE_IRQ, k-1);
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@...r.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/