Re: [PATCH] i386: Check invalid (%dx) usage

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Nov 7, 2022 at 11:34 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.11.2022 20:58, H.J. Lu wrote:
> > On Mon, Nov 7, 2022 at 3:44 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 07.11.2022 10:55, Jan Beulich via Binutils wrote:
> >>> On 04.11.2022 21:55, H.J. Lu via Binutils wrote:
> >>>> (%dx) isn't a valid memory address in any modes.  It is used as a special
> >>>> memory operand for input/output port address in AT&T syntax and should
> >>>> only be used with input/output instructions.  Update i386_att_operand to
> >>>> set i.input_output_operand to true for (%dx) and issue an error if (%dx)
> >>>> is used with non-input/output instructions.
> >>>
> >>> Hmm, this shouldn't require a new flag I would hope. We did properly reject
> >>> bad uses up to 2.31 ("operand type mismatch"). Whatever was broken there
> >>> would need correcting instead, imo. A possible candidate looks to be
> >>> 2fb5be8dac9d ("x86: drop {,reg16_}inoutportreg variables"), albeit perhaps
> >>> combined with later changes - in 2.33 behavior changed again.
> >>
> >> What about the change below, perhaps combined with your testsuite adjustments
> >> (albeit I'd like to point out that "incl" isn't the best choice, as %dx is
> >
> > Since incl is misassembled, it is a good test.
>
> Well, perhaps both incl and incw then?

Sure.

> >> invalid with that anyway; "incw" would be better)? That way we'll uniformly
> >> get "`(%dx)' is not a valid base/index expression" for bad uses of (%dx),
> >> matching any other uses of wrong addressing forms.
> >>
> >> Jan
> >>
> >> x86: restrict use of (%dx)
> >>
> >> PR gas/29751
> >> The AT&T mode special case operand (%dx) is valid to use only with
> >> instructions nominally expecting %dx to specify an I/O port address.
> >> Prefix the respective checking with an opcode check. Keep that as
> >> simple as possible by recognizing that opcodes 0x64 and 0x66 (which
> >
> > Since current_templates doesn't point to the matched instruction,
> > checking current_templates looks like abuse.  I don't think error
> > messages should be a concern here.
>
> We use current_templates in similar ways in quite a number of places,
> when match_templates() hasn't run yet.

Since the first template isn't the selected one, your check allows
the invalid opcodes.

> >> wrongly also match the check) encode prefixes, which hence - even if
> >> used standalone - don't take any operands, so match_template() will
> >> fail there for other reasons.
> >>
> >> While there also complete the transformation from memory to register
> >
> > I prefer to keep it ASIS since the lack of the transformation helped
> > catch this error.
>
> Interesting view. I think with just this code added you'd still see
> mis-assembly; you merely wouldn't see ...

True since it uses the invalid template.

> >> operand: The lack thereof was responsible for SEGV when (%dx) was
> >> (wrongly) used with certain insns.
>
> ... SEGV anymore.
>
> Jan



-- 
H.J.