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

[Masami Hiramatsu <mhiramat@kernel.org>]

On Fri, 4 Dec 2020 16:05:22 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Fri, Dec 04, 2020 at 07:55:20PM +0900, Masami Hiramatsu wrote:
> > +/**
> > + * for_each_insn_prefix() -- Iterate prefixes in the instruction
> > + * @insn: Pointer to struct insn.
> > + * @idx:  Index storage.
> > + * @prefix: Prefix byte.
> > + *
> > + * Iterate prefix bytes of given @insn. Each prefix byte is stored in @prefix
> > + * and the index is stored in @idx (note that this @idx is just for a cursor,
> > + * do not change it.)
> > + * Since prefixes.nbytes can be bigger than NUM_INSN_FIELD_BYTES when some
> > + * prefixes are repeated, it can not be used for looping over the prefixes.
> > + */
> > +#define for_each_insn_prefix(insn, idx, prefix)				\
> > +	for (idx = 0;							\
> > +	     idx < MAX_LEGACY_PREFIX_GROUPS &&				\
> 
> The problem I see here is that you check for the index limit to be
> < MAX_LEGACY_PREFIX_GROUPS but the array itself is defined using
> NUM_INSN_FIELD_BYTES, and that is confusing.

Yeah, I considered that once. If I know the number of legacy prefix
groups never exceed the size of prefixes.bytes, then we would better
use the max number of legacy prefix here (because we are looping on
the bytes from the prefix groups).
That is why I added #error check in this patch.

> I guess this should be:
> 
> #define MAX_LEGACY_PREFIX_GROUPS	4
> #define NUM_INSN_FIELD_BYTES		MAX_LEGACY_PREFIX_GROUPS
> 
> and later, iff the legacy prefixes array size needs separating from the
> insn field array size, then the defines would need to change too.

No, those have different meaning. NUM_INSN_FIELD_BYTES means
sizeof(s32) / sizeof(u8), which comes from the definition of
insn_field data structure. But MAX_LEGACY_PREFIX_GROUPS comes
from the x86 ISA.

In the future, if x86 ISA is expanded and add a legacy prefix
groups, then we have to add new insn_prefix_field data structure,
which size will not depend on NUM_INSN_FIELD_BYTES, but still
depend on MAX_LEGACY_PREFIX_GROUPS (and that will be 5).

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>