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

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

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.

> 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.

> 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.

> operand: The lack thereof was responsible for SEGV when (%dx) was
> (wrongly) used with certain insns.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11884,7 +11884,9 @@ i386_att_operand (char *operand_string)
>         }
>
>        /* Special case for (%dx) while doing input/output op.  */
> -      if (i.base_reg
> +      if ((current_templates->start->base_opcode | 0x8a) == 0xee
> +         && current_templates->start->opcode_modifier.opcodespace == SPACE_BASE
> +         && i.base_reg
>           && i.base_reg->reg_type.bitfield.instance == RegD
>           && i.base_reg->reg_type.bitfield.word
>           && i.index_reg == 0
> @@ -11893,6 +11895,8 @@ i386_att_operand (char *operand_string)
>           && !operand_type_check (i.types[this_operand], disp))
>         {
>           i.types[this_operand] = i.base_reg->reg_type;
> +         i.op[this_operand].regs = i.base_reg;
> +         i.reg_operands++;
>           return 1;
>         }
>
>
>


-- 
H.J.