linux-kernel - Re: [PATCH v2] ARM: kprobes: rewrite test-[arm|thumb].c in UAL

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: <CAKwvOdkv9714jdhHuc7o90TY_sOKSe1J+NmwcxBxEaX-haJk0w@mail.gmail.com>
Date:   Mon, 29 Mar 2021 15:08:45 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Russell King <linux@...linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        Nathan Chancellor <natechancellor@...il.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>,
        Stephen Hines <srhines@...gle.com>,
        David Spickett <david.spickett@...aro.org>
Subject: Re: [PATCH v2] ARM: kprobes: rewrite test-[arm|thumb].c in UAL

On Fri, Jan 29, 2021 at 1:40 AM Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Fri, 29 Jan 2021 at 01:22, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
> >
> > > On Thu, 28 Jan 2021 at 20:34, Nick Desaulniers <ndesaulniers@...gle.com> wrote:
> > > > +       TEST_RX("tbh    [pc, r",7, (9f-(1f+4))>>1,", lsl #1]",
> > > >
> > > On Thu, Jan 28, 2021 at 1:03 PM Ard Biesheuvel <ardb@...nel.org> wrote:
> > > Why is this change needed? Are the resulting opcodes equivalent? Does
> > > GAS infer the lsl #1 but Clang doesn't?
> >
> > Yes; it seems if you serialize/deserialize this using GNU `as` and
> > objdump, that's the canonical form (GNU objdump seems to print in UAL
> > form, IIUC).  I didn't see anything specifically about `tbh` in
> > https://developer.arm.com/documentation/dui0473/c/writing-arm-assembly-language/assembly-language-changes-after-rvctv2-1?lang=en
> > but it's what GNU objdump produces and what clang's integrated
> > assembler accepts.
> >
>
> This matches the ARM ARM: TBB and TBH are indexed table lookups, where
> the table consists of 16-bit quantities in the latter case, and so the
> LSL #1 is implied.

(Sorry, more time to look into thumb2 now, revisiting this)

Then I would have expected it to be stated that the operand is
implicit in the Arm ARM or use the curly brace notation for the
optional operands (see "F5.1.167 RSB, RSBS (register)" for an example
of what I'm talking about. AFAICT, there's no such language in the Arm
ARM about TBH. This is also what GNU binutils' objdump spits out when
disassembled.  And TBB differs from TBH in regards to this operand;
TBH has it, TBB does not.  It's not clear to me whether these
shorthands are intentional (pseudo ops) vs unintentional (parsing
bugs) in GAS.

>
> > > >
> > > >  #define _DATA_PROCESSING32_DNM(op,s,val)                                       \
> > > > -       TEST_RR(op s".w r0,  r",1, VAL1,", r",2, val, "")                       \
> > > > +       TEST_RR(op s"   r0,  r",1, VAL1,", r",2, val, "")                       \
> > >
> > > What is wrong with these .w suffixes? Shouldn't the assembler accept
> > > these even on instructions that only exist in a wide encoding?
> >
> > Yeah, I'm not sure these have anything to do with UAL.  Looking at
> > LLVM's sources and IIRC, LLVM has "InstAlias"es it uses for .w
> > suffixes. I think I need to fix those in LLVM for a couple
> > instructions, rather than modify these in kernel sources.  I'll split
> > off the arm-test.c and thumb-test.c into separate patches, fix LLVM,
> > and drop the .w suffix changes to thumb-test.c.
> >
>
> The ARM ARM (DDI0487G.a F1.2) clearly specifies that any instruction
> documented as supporting the optional {q} suffix (which is almost all
> if not all of them) can be issued with the .w appended, even if doing
> so is redundant (note that it even documents this as being supported
> for the A32 ISA, which GAS does not implement today either)
>
> Given how we often use this suffix to ensure that the opcode fills the
> expected amount of space (for things like jump tables, etc), we cannot
> simply drop these left and right and expect things to still work.

I understand fixed in:
* BL+DBG:   https://reviews.llvm.org/D97236
* ORN/ORNS: https://reviews.llvm.org/D99538
* RSB/RSBS: https://reviews.llvm.org/D99542

-- 
Thanks,
~Nick Desaulniers