linux-kernel - Re: [PATCH net v2] net: mdio-ipq4019: fix possible invalid pointer dereference

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: <Y3eMMc7maaPCKUNS@lunn.ch>
Date:   Fri, 18 Nov 2022 14:44:17 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Hui Tang <tanghui20@...wei.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        mw@...ihalf.com, linux@...linux.org.uk, pabeni@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        yusongping@...wei.com
Subject: Re: [PATCH net v2] net: mdio-ipq4019: fix possible invalid pointer
 dereference

> So, the code should be as follows, is that right?
> 
> +	void __iomem *devm_ioremap_resource_optional(struct device *dev,
> +                                    	     const struct resource *res)
> +	{
> +		void __iomem *base;
> +
> +		base = __devm_ioremap_resource(dev, res, DEVM_IOREMAP);
> +		if (IS_ERR(base) && PTR_ERR(base) == -ENOMEM)
> +			return NULL;
> +
> +		return base;
> +	}
> 
> 
> [...]
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (res)
> +	if (res) {
> +		priv->eth_ldo_rdy = devm_ioremap_resource_optional(&pdev->dev, res)
> +		if (IS_ERR(priv->eth_ldo_rdy))
> +			return PTR_ERR(priv->eth_ldo_rdy);
> +	}
> [...]

Yes, that is the basic concept.

The only thing i might change is the double meaning of -ENOMEM.
__devm_ioremap_resource() allocates memory, and if that memory
allocation fails, it returns -ENOMEM. If the resource does not exist,
it also returns -ENOMEM. So you cannot tell these two error conditions
apart. Most of the other get_foo() calls return -ENODEV if the
gpio/regulator/clock does not exist, so you can tell if you are out of
memory. But ioremap is specifically about memory so -ENOMEM actually
makes sense.

If you are out of memory, it seems likely the problem is not going to
go away quickly, so the next allocation will also fail, and hopefully
the error handling will then work. So i don't think it is major
issue. So yes, go with the code above.

      Andrew