Re: [PATCH v7 2/5] x86/asm: add volatile, clobbers and zero-length check in inline memcmp

[Brian Gerst <brgerst@xxxxxxxxx>]

On Wed, Jul 22, 2026 at 2:53 PM H. Peter Anvin <hpa@xxxxxxxxx> wrote:
>
> On July 22, 2026 10:03:34 AM PDT, Borislav Petkov <bp@xxxxxxxxx> wrote:
> >hpa in To:.
> >
> >On Tue, Jul 21, 2026 at 12:56:43PM -0300, Mauricio Faria de Oliveira wrote:
> >> Add the volatile qualifier and clobbers parameter to prevent bugs with
> >> instruction reordering and optimization.
> >>
> >> Also check the zero-length case, as the 'repe' prefix does not run the
> >> 'cmpsb' instruction if the 'count' register is zero, which doesn't set
> >
> >Please use capital letters for insns: REPE, CMPSB and you don't need to put
> >words in '' - it reads fine without them.
> >
> >> the condition-code/zero flag, so the result is based on a stale flag.
> >>
> >> Those are pre-existing issues found by Sashiko.
> >>
> >> Link: 
> >> https://sashiko.dev/#/patchset/20260701-pvh-kasan-inline-v6-0-ba99045dfa9f%40igalia.com
> >> Signed-off-by: Mauricio Faria de Oliveira <mfo@xxxxxxxxxx>
> >> ---
> >>  arch/x86/include/asm/shared/string.h | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/shared/string.h 
> >> b/arch/x86/include/asm/shared/string.h
> >> index 
> >> 02b92927553f7b8e1c87e6122bbaa70439e57ea7..166274e44f3cb49e3dccab3cdac281d67aef5d44
> >>  100644
> >> --- a/arch/x86/include/asm/shared/string.h
> >> +++ b/arch/x86/include/asm/shared/string.h
> >> @@ -11,8 +11,12 @@ static __always_inline int __inline_memcmp(const void 
> >> *s1, const void *s2, size_
> >>  {
> >>      bool diff;
> >>
> >> -    asm("repe cmpsb"
> >> -        : "=@ccnz" (diff), "+D" (s1), "+S" (s2), "+c" (len));
> >> +    if (len == 0)
> >> +            return 0;
> >> +
> >> +    asm volatile("repe cmpsb"
> >> +                 : "=@ccnz" (diff), "+D" (s1), "+S" (s2), "+c" (len)
> >> +                 : : "cc", "memory");
> >>
> >>      return diff;
> >>  }
> >
> >So this is a fix which should probably go to stable, I think. Going back into
> >git history, it points to
> >
> >  62bd0337d0c4 ("Top header file for new x86 setup code")
> >
> >from 2007. And we have carried it this way through the years and who knows
> >what hit this or not. So please make this the first patch in your set - you
> >can even send it separately so that I can get route it through stable.
> >
> >Then you can base the rest ontop.
> >
> >Thx.
> >
>
> Also, this is silly. Instead of adding a whole separate test, just do "test 
> %3,%3" before the repe to set ZF and let the REPE skip.

REPE does not check ZF before the first iteration.

>From the Intel SDM:
"When the REPE/REPZ and REPNE/REPNZ prefixes are used, the ZF flag
does not require initialization because both
the CMPS and SCAS instructions affect the ZF flag according to the
results of the comparisons they make."