Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

[Masami Hiramatsu <mhiramat@kernel.org>]

On Fri, 4 Dec 2020 09:56:53 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 3 Dec 2020 12:49:46 -0600
> Tom Lendacky <thomas.lendacky@amd.com> wrote:
> 
> > On 12/3/20 12:17 PM, Borislav Petkov wrote:
> > > On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote:
> > >> Since that struct is used in multiple places, I think basing it on the array
> > >> size is the best way to go. The main point of the check is just to be sure
> > >> you don't read outside of the array.
> > > 
> > > Well, what happens if someone increases the array size of:
> > > 
> > > struct insn_field {
> > > 	union {
> > > 		insn_byte_t bytes[4];
> > > 				^^^^
> > > 
> > > ?
> > 
> > I think we need to keep the parsing of the instruction separate from 
> > accessing the prefixes after (successfully) parsing it. This fix is merely 
> > making sure that we don't read outside the bounds of the array that 
> > currently holds the legacy prefixes.
> > 
> > > 
> > > That's why a separate array only for legacy prefixes would be better
> > > in the long run. The array size check is good as a short-term fix for
> > > stable.
> > > 
> > > I'd say.
> > 
> > According to Volume 3 of the AMD APM (Figure 1-2 on page 5), there could 
> > be as many as 5 legacy prefixes and it says that more than one prefix from 
> > each group is undefined behavior. The instruction parsing code doesn't 
> > seem to take into account the different prefix groups. So I agree with you 
> > that short term the array size check works, and long term, the legacy 
> > prefix support probably needs a closer look.
> 
> Hmm, there is a difference between Intel SDM and AMD APM.
> 
> Intel SDM vol.2
> 
> 2.1.1 Instruction Prefixes
> Instruction prefixes are divided into four groups, each with a set of allowable prefix codes. For each instruction, it
> is only useful to include up to one prefix code from each of the four groups (Groups 1, 2, 3, 4).
> 
> AMD APM vol.3
> 
> 1.2.1 Summary of Legacy Prefixes
> Table 1-1 on page 7 shows the legacy prefixes. The legacy prefixes are organized into five groups, as
> shown in the left-most column of Table 1-1. An instruction encoding may include a maximum of one
> prefix from each of the five groups.
> 
> So, Intel CPU doesn't accept LOCK-REP because those are in a same prefix
> group, but AMD says it is acceptable. Actually, insn.c only accepts the 
> prefix up to 4, so if there is any instruction which has 5 prefixes,
> it will fail to parse.


OK, I got it. AMD APM's explanation is misleading. Intel SDM groups
the legacy prefixes in 1) Lock/Repeat/Bound 2) Segment override/branch hints,
3) Operand-size override 4) Address-size override. On the other hand,
AMD APM makes it 5 groups(See Table 1-1), 1) Operand-Size Override
2) Address-Size Override 3) Segment Override 4) Lock 5) Repeat.

So the difference is whether the Lock and Repeat is a group or not.

However, I found we must not see LOCK-REP prefix in the same instruction,
because the available instruction for LOCK and REP are different.

AMD APM vol.3
---
1.2.5 Lock Prefix
...
The LOCK prefix can only be used with forms of the following instructions that write a memory
operand: ADC, ADD, AND, BTC, BTR, BTS, CMPXCHG, CMPXCHG8B, CMPXCHG16B, DEC,
INC, NEG, NOT, OR, SBB, SUB, XADD, XCHG, and XOR. An invalid-opcode exception occurs if
the LOCK prefix is used with any other instruction.

1.2.6 Repeat Prefixes
The repeat prefixes cause repetition of certain instructions that load, store, move, input, or output
strings. The prefixes should only be used with such string instructions.
...
REP. ... The prefix can be used with the INS, LODS, MOVS, OUTS, and STOS instructions.
...
The REPE and REPZ prefixes can be used with the CMPS, CMPSB, CMPSD, CMPSW, SCAS,
SCASB, SCASD, and SCASW instructions.
...
The REPNE and REPNZ prefixes can be used with the CMPS, CMPSB, CMPSD, CMPSW, SCAS,
SCASB, SCASD, and SCASW instructions.
---

Thus, I think the current expectation -- legacy prefix will consist
of 4 different groups -- is good. No need to take care of LOCK-REP
combination.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>