linux-kernel - Re: net: dsa: sja1105: use detected device id instead of DT one on mismatch

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] [day] [month] [year] [list]

Date:   Thu, 6 Aug 2020 22:25:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: net: dsa: sja1105: use detected device id instead of DT one on
 mismatch

On Thu, Aug 06, 2020 at 05:27:11PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected a potential issue with the
> following commit:
> 
> commit 0b0e299720bb99428892a23ecbd2b4b7f61ccf6d
> Author: Vladimir Oltean <olteanv@...il.com>
> Date:   Mon Aug 3 19:48:23 2020 +0300
> 
>     net: dsa: sja1105: use detected device id instead of DT one on mismatch
> 
> The analysis is as follows:
> 
> Array compared against 0 (NO_EFFECT)array_null: Comparing an array to
> null is not useful: match->compatible, since the test will always
> evaluate as true.
> 
>     Was match->compatible formerly declared as a pointer?
> 
> 3418        for (match = sja1105_dt_ids; match->compatible; match++) {
> 3419                const struct sja1105_info *info = match->data;
> 3420
> 
> I'm not sure what the original intention was, so I was unable to fix
> this hence I'm sending this report as I think it needs addressing.
> 
> Colin

The intention was to loop through sja1105_dt_ids and stop at the
sentinel:

static const struct of_device_id sja1105_dt_ids[] = {
	{ .compatible = "nxp,sja1105e", .data = &sja1105e_info },
	{ .compatible = "nxp,sja1105t", .data = &sja1105t_info },
	{ .compatible = "nxp,sja1105p", .data = &sja1105p_info },
	{ .compatible = "nxp,sja1105q", .data = &sja1105q_info },
	{ .compatible = "nxp,sja1105r", .data = &sja1105r_info },
	{ .compatible = "nxp,sja1105s", .data = &sja1105s_info },
	{ /* sentinel */ },
};

I should have looked at the definition of struct of_device_id:

/*
 * Struct used for matching a device
 */
struct of_device_id {
	char	name[32];
	char	type[32];
	char	compatible[128];
	const void *data;
};

Honestly, I had thought it's "const char *compatible" rather than "char
compatible[128]". I'm still not 100% clear why it isn't doing just that,
though, I think it has to do with some weird usage patterns such as this
one in UIO:

static struct of_device_id uio_of_genirq_match[] = {
	{ /* This is filled with module_parm */ },
	{ /* Sentinel */ },
};
MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");

So I had 2 options for this patch: either break the loop on
match->compatible, or on match->data. And it looks like I made the wrong
one.

Thanks,
-Vladimif