linux-kernel - Re: [PATCH] arm64: Remove d-cache clean operation at preserve_boot

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

Message-ID: <CAMguKxboV7iVz6J77cZZcrKv-q6QprO1THE-gnXv-ourf1GKoA@mail.gmail.com>
Date:   Tue, 6 Sep 2022 14:35:02 +0200
From:   Jeungwoo Yoo <casionwoo@...il.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Hyeonggon Yoo <42.hyeyoo@...il.com>,
        Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>,
        Sangyun Kim <sangyun.kim@....ac.kr>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: Remove d-cache clean operation at preserve_boot_args().

2022년 9월 5일 (월) 오후 12:47, Robin Murphy <robin.murphy@....com>님이 작성:
>
> On 2022-09-04 20:30, Jeungwoo Yoo wrote:
> > Kernel expects only the clean operation as a booting requirement in
> > arm64 architecture [1], therefore, the kernel has to invalidate any
> > cache entries after accessing a memory in the booting time (before
> > enabling D-cache and MMU) not to overwrite the memory with the stale
> > cache entry.
> >
> > Same applied in preserve_boot_args(), kernel saves boot arguments into
> > 'boot_args' and invalidates the corresponding cache entry. However,
> > according to the 'dcache_inval_poc()' implementation, the cache entry
> > will be not only invalidated but also cleaned. That means if there is a
> > stale cache entry corresponding to the address of the 'boot_args', the
> > saved boot arguments in 'boot_args' will be overwritten by the stale
> > cache entry. Therefore, it uses 'dv ivac' instruction directly instead
> > of calling 'dcache_inval_poc()'.
>
> You've already said in the first paragraph that we expect these
> locations to be clean. Clean lines are not written back, so your
> reasoning here is spurious. If boot_args has somehow become dirtied such
> that the clean operation *would* write back to memory, that can only
> mean one of two things: either the kernel image was not cleaned per the
> boot protocol, in which case there's every chance that other things will
> also go wrong elsewhere and there's not much we can do, or the prior
> stores hit in the cache (either unexpectedly or because the MMU was left
> on), in which case we almost certainly *would* want writeback here anyway.

Yes, you are right. I misunderstood that the "clean operation" will
propagate the content from the d-cache to the memory always.
However, as the cache line is already cleaned by the bootloader and
not modified in the d-cache, the "clean operation" in
"dcache_inval_poc()" will be ignored.
Thank you for your correction.

>
> > The address of the 'boot_args' is aligned to the cache line size and the
> > size of 'boot_args' is 32 byte (8 byte * 4), therefore, a single
> > invalidate operation is enough to invalidate the cache line belonging to
> > the 'boot_args'.
>
> The architecture allows the CWG to be as small as 2 words, so this is
> clearly untrue.
>
> Thanks,
> Robin.
>
> > Sometimes clean operation is required not to lose any contents in the
> > cache entry but not the target of the invalidation. However, in this
> > case, there is no valid cache entries at a very early booting stage and
> > preserve_boot_args() is not called by any other (non-primary) CPUs.
> > Therefore, this invalidation operation will not introduce any problems.
> >
> > [1] in Documentation/arm64/booting.rst:
> > The address range corresponding to the loaded kernel image must be
> > cleaned to the PoC.
> >
> > Co-developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@...el.com>
> >
> > Co-developed-by: Sangyun Kim <sangyun.kim@....ac.kr>
> > Signed-off-by: Sangyun Kim <sangyun.kim@....ac.kr>
> >
> > Signed-off-by: Jeungwoo Yoo <casionwoo@...il.com>
> > ---
> >   arch/arm64/kernel/head.S | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index cefe6a73ee54..916227666b07 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -121,9 +121,7 @@ SYM_CODE_START_LOCAL(preserve_boot_args)
> >
> >       dmb     sy                              // needed before dc ivac with
> >                                               // MMU off
> > -
> > -     add     x1, x0, #0x20                   // 4 x 8 bytes
> > -     b       dcache_inval_poc                // tail call
> > +     dc      ivac, x0                        // Invalidate potentially stale cache line
> >   SYM_CODE_END(preserve_boot_args)
> >
> >   SYM_FUNC_START_LOCAL(clear_page_tables)



-- 
Best Regards,
Jeungwoo Yoo