linux-kernel - Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32

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:   Sat, 29 Aug 2020 10:29:55 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Luc Van Oostenryck <luc.vanoostenryck@...il.com>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Joerg Roedel <joerg.roedel@....com>,
        Guenter Roeck <linux@...ck-us.net>,
        Li Yang <leoyang.li@....com>, Zhang Wei <zw@...kernel.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Vinod Koul <vkoul@...nel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        dma <dmaengine@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()

On Sat, Aug 29, 2020 at 5:46 AM Luc Van Oostenryck
<luc.vanoostenryck@...il.com> wrote:
>
> But the pointer is already 32-bit, so simply cast the pointer to u32.

Yeah, that code was completely pointless. If the pointer had actually
been 64-bit, the old code would have warned too.

The odd thing is that the fsl_iowrite64() functions make sense. It's
only the fsl_ioread64() functions that seem to be written by somebody
who is really confused.

That said, this patch only humors the confusion. The cast to 'u32' is
completely pointless. In fact, it seems to be actively wrong, because
it means that the later "fsl_addr + 1" is done entirely incorrectly -
it now literally adds "1" to an integer value, while the iowrite()
functions will add one to a "u32 __iomem *" pointer (so will do
pointer arithmetic, and add 4).

So this code has never ever worked correctly to begin with, but the
patches to fix the warning miss the point. The problem isn't the
warning, the problem is that the code is broken and completely wrong
to begin with.

And the "lower_32_bits()" thing has always been pure and utter
confusion and complete garbage.

I *think* the right patch is the one attached, but since this code is
clearly utterly broken, I'd want somebody to test it.

It has probably never ever worked on 32-bit powerpc, or did so purely
by mistake (perhaps because nobody really cares - the only 64-bit use
is this:

    static dma_addr_t get_cdar(struct fsldma_chan *chan)
    {
        return FSL_DMA_IN(chan, &chan->regs->cdar, 64) & ~FSL_DMA_SNEN;
    }

and there are two users of that: one which ignores the return value,
and one that looks like it might end up half-way working even if the
value read was garbage (it's used only to compare against a "current
descriptor" value).

Anyway, the fix is definitely not to just shut up the warning. The
warning is only a sign of utter confusion in that driver.

Can somebody with the hardware test this on 32-bit ppc?

And if not (judging by just how broken those functions are, maybe it
never did work), can somebody with a ppc32 setup at least compile-test
this patch and look at whether it makes sense, in ways the old code
did not.

                Linus

Download attachment "patch" of type "application/octet-stream" (1167 bytes)