linux-kernel - Re: [PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of

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: <87h738xkhw.wl-maz@kernel.org>
Date:   Sat, 23 Jul 2022 10:10:19 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     William Dean <williamsukatube@...il.com>
Cc:     williamsukatube@....com, tglx@...utronix.de,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org,
        tsbogend@...ha.franken.de, fancer.lancer@...il.com,
        Hacash Robot <hacashRobot@...tino.com>
Subject: Re: [PATCH] irqchip: mips-gic: check the return value of ioremap() in gic_of_init()

On Sat, 23 Jul 2022 03:57:18 +0100,
William Dean <williamsukatube@...il.com> wrote:
> 
> Thans for your advice.
> 
> > Erm... No. The issue was definitely there before (just look at the
> > patch you quote here).
> 
> Do you mean that I wrote the fixes-tag wrong?  I just checked it, it was
> introduced by the earlier commit fbea754123ae5d9678295398c98e91f1b2159e5b,
> if you mean that, I will send patch v2

Not even that. Here's what this patch has:

-       mips_gic_base = ioremap_nocache(gic_base_addr, gic_addrspace_size);
+       if (mips_cm_present()) {
+               write_gcr_gic_base(gic_base | CM_GCR_GIC_BASE_GICEN);
+               /* Ensure GIC region is enabled before trying to access it */
+               __sync();
+       }
+
+       mips_gic_base = ioremap_nocache(gic_base, gic_len);

        gicconfig = read_gic_config();
        gic_shared_intrs = gicconfig & GIC_CONFIG_NUMINTERRUPTS;

Still no check for the ioremap result. You can actually trace it all
the way down to 39b8d5254246a ("[MIPS] Add support for MIPS CMP
platform."), which introduced that code initially. But is it worth
referencing such an old commit? No.

To be honest, given how early this code executes, failing to ioremap
something is utterly unlikely, and adding the check is mostly about
being pedantic.

It is also worth noting that most of the use of this variable are
guarded by mips_gic_present(), which actually checks for mips_gic_base
being NULL and that the driver actually *ignores* such mapping.

Given that, I'm not sure this deserves an actual Fixes: tag. This is
completely harmless. Just resend the patch with a correct SoB, and
I'll queue it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.