linux-kernel - Re: [PATCH] iommu/omap: Use helper function IS_ERR_OR

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

Message-ID: <ZPn8oKc2s0HeBw1A@ziepe.ca>
Date:   Thu, 7 Sep 2023 13:38:56 -0300
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Bo Liu <liubo03@...pur.com>
Cc:     joro@...tes.org, will@...nel.org, robin.murphy@....com,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/omap: Use helper function IS_ERR_OR_NULL()

On Thu, Sep 07, 2023 at 03:14:09AM -0400, Bo Liu wrote:
> Use IS_ERR_OR_NULL() to detect an error pointer or a null pointer
> open-coding to simplify the code.
> 
> Signed-off-by: Bo Liu <liubo03@...pur.com>
> ---
>  drivers/iommu/omap-iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Please no, IS_ERR_OR_NULL is an abomination.

There are only two callers:

	for (i = 0; i < omap_domain->num_iommus; i++, iommu--, arch_data--) {
		oiommu = iommu->iommu_dev;
		iopgtable_clear_entry_all(oiommu);

		omap_iommu_detach(oiommu);

Obviously oiommu is not NULL or ERR since we derefed it

The second:

attach_fail:
	while (i--) {
		iommu--;
		arch_data--;
		oiommu = iommu->iommu_dev;
		omap_iommu_detach(oiommu);
		iommu->iommu_dev = NULL;
		oiommu->domain = NULL;

And here I don't see how iomm->iommu_dev can ever be NULL or
ERR_PTR. The i-- follows this:

	iommu = omap_domain->iommus;
	for (i = 0; i < omap_domain->num_iommus; i++, iommu++, arch_data++) {
		/* configure and enable the omap iommu */
		oiommu = arch_data->iommu_dev;
		ret = omap_iommu_attach(oiommu, iommu->pgtable);
		if (ret) {
			dev_err(dev, "can't get omap iommu: %d\n", ret);
			goto attach_fail;
		}

		oiommu->domain = domain;
		iommu->iommu_dev = oiommu;

And again we have always deref'd iommu->iommu_dev.

It is just wrong defensive coding, remove it.

Jason