linux-kernel - Re: [PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev

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:   Wed, 27 Apr 2022 10:13:21 +0800
From:   Miles Chen <miles.chen@...iatek.com>
To:     <yong.wu@...iatek.com>
CC:     <angelogioacchino.delregno@...labora.com>,
        <hverkuil-cisco@...all.nl>, <iommu@...ts.linux-foundation.org>,
        <joro@...tes.org>, <jroedel@...e.de>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <mchehab@...nel.org>, <miles.chen@...iatek.com>,
        <robin.murphy@....com>, <will@...nel.org>
Subject: Re: [PATCH v2] iommu/mediatek: fix NULL pointer dereference when printing dev_name

Hi Yong,

>On Mon, 2022-04-25 at 11:03 +0100, Robin Murphy wrote:
>> On 2022-04-25 09:24, Miles Chen via iommu wrote:
>> > When larbdev is NULL (in the case I hit, the node is incorrectly
>> > set
>> > iommus = <&iommu NUM>), it will cause device_link_add() fail and
>> 
>> Until the MT8195 infra MMU support lands, is there ever a case where 
>> it's actually valid for larbdev to be NULL? If not, I think it would
>> be 
>> a lot clearer to explicitly fail the probe here, rather than
>> silently 
>> continue and risk fatal errors, hangs, or other weird behaviour if 
>> there's no guarantee that the correct LARB is powered up (plus then
>> the 
>> release callbacks wouldn't need to worry about it either).
>
>Yes. It should return fail for this case. This issue only happens when
>the dts parameters doesn't respect the definition from the binding[1].
>
>Locally Miles tested with a internal definition that have not send
>upstream to get this KE. In this case, I'm not sure if we should
>request the user use the right ID in dts. Anyway I have no objection to
>modifying this, then something like this: (Avoid invalid input from
>dtb)
>
>@@ -790,6 +790,8 @@ static struct iommu_device
>*mtk_iommu_probe_device(struct device *dev)
> 	 * All the ports in each a device should be in the same larbs.
> 	 */
> 	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
>+	if (larbid >= MTK_LARB_NR_MAX)
>+		return ERR_PTR(-EINVAL);
> 	for (i = 1; i < fwspec->num_ids; i++) {
> 		larbidx = MTK_M4U_TO_LARB(fwspec->ids[i]);
> 		if (larbid != larbidx) {
>@@ -799,6 +801,8 @@ static struct iommu_device
>*mtk_iommu_probe_device(struct device *dev)
> 		}
> 	}
> 	larbdev = data->larb_imu[larbid].dev;
>+	if (!larbdev)
>+		return ERR_PTR(-EINVAL);
> 	link = device_link_add(dev, larbdev,
> 			       DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS);
> 	if (!link)

Thanks for guilding me, I will put this in patch v2.

Thanks,
Miles

>
>
>[1] 
>https://elixir.bootlin.com/linux/v5.18-rc1/source/Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml#L116