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]

Date:	Thu, 18 Feb 2010 21:02:19 +0100
From:	roel kluin <roel.kluin@...il.com>
To:	"Karicheri, Muralidharan" <m-karicheri2@...com>
Cc:	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

>>-      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 don't see how std_info could get updated. consider the loop in case
std_info->stdid equals 0:

        for (index = 0; index < ARRAY_SIZE(ch_params); index++) {
                config = &ch_params[index];

(config is a local variable)

                if (config->stdid & std_info->stdid) {

This fails for every index if std_info->stdid equals 0.

                        memcpy(std_info, config, sizeof(*config));
                        break;
                }
        }

So we always reach this with index == ARRAY_SIZE(ch_params)

        if (index == ARRAY_SIZE(ch_params))
                return -1;

So we could have returned -1 earlier.

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

>>+      if (k == VPIF_DISPLAY_MAX_DEVICES)
>>+              k = VPIF_DISPLAY_MAX_DEVICES - 1;

actually I think this is still not right. shouldn't it be be

k = VPIF_DISPLAY_MAX_DEVICES - 1;

> are you using this driver in your project?

No, I just found this in the code.

Thanks, Roel
--
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/