x86: Check invalid immediate for rdmsr and wrmsrns

  Verify immediate operand before processing immediate for rdmsr and
wrmsrns.

PR gas/34028
* config/tc-i386.c (i386_assemble): Verify immediate operand
before processing immediate for rdmsr and wrmsrns.
* testsuite/gas/i386/msr_imm-inval.l: Updated.
* testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
* testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
tests.
* testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
  

On 08.04.2026 02:22, H.J. Lu wrote:
> Verify immediate operand before processing immediate for rdmsr and
> wrmsrns.
> 
> PR gas/34028
> * config/tc-i386.c (i386_assemble): Verify immediate operand
> before processing immediate for rdmsr and wrmsrns.
> * testsuite/gas/i386/msr_imm-inval.l: Updated.
> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
> tests.
> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.

The new insn forms you add don't make clear what exactly you're after:
The %cs operands make them invalid no matter what immediate is used. I
don't mind you adding "bogus register" forms, but that's not related
to the purpose of the patch. $y, otoh, looks like an entirely valid
operand to me: y can be an absolute symbol defined elsewhere. So what
exactly is it that you're trying to (a) prevent and (b) test?

As an aside, adding further insn forms to the 32-bit testcase looks
pretty meaningless to me. The forms with operands aren't supported
outside of 64-bit mode anyway, no matter what exactly the operands
are.

Jan
  

On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.04.2026 02:22, H.J. Lu wrote:
> > Verify immediate operand before processing immediate for rdmsr and
> > wrmsrns.
> >
> > PR gas/34028
> > * config/tc-i386.c (i386_assemble): Verify immediate operand
> > before processing immediate for rdmsr and wrmsrns.
> > * testsuite/gas/i386/msr_imm-inval.l: Updated.
> > * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
> > * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
> > tests.
> > * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
>
> The new insn forms you add don't make clear what exactly you're after:
> The %cs operands make them invalid no matter what immediate is used. I
> don't mind you adding "bogus register" forms, but that's not related
> to the purpose of the patch. $y, otoh, looks like an entirely valid
> operand to me: y can be an absolute symbol defined elsewhere. So what
> exactly is it that you're trying to (a) prevent and (b) test?
>
> As an aside, adding further insn forms to the 32-bit testcase looks
> pretty meaningless to me. The forms with operands aren't supported
> outside of 64-bit mode anyway, no matter what exactly the operands
> are.

Assembler crashed at:

      if (is_cpu(current_templates.start, CpuUSER_MSR)
          || t->mnem_off == MN_rdmsr
          || t->mnem_off == MN_wrmsrns)
        {
          for (j = 0; j < i.imm_operands; j++)
            i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        }

My patch prevents segfault.  The new testcases caused segfault in
both 32-bit and 64-bit modes.
  

On 08.04.2026 09:27, H.J. Lu wrote:
> On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.04.2026 02:22, H.J. Lu wrote:
>>> Verify immediate operand before processing immediate for rdmsr and
>>> wrmsrns.
>>>
>>> PR gas/34028
>>> * config/tc-i386.c (i386_assemble): Verify immediate operand
>>> before processing immediate for rdmsr and wrmsrns.
>>> * testsuite/gas/i386/msr_imm-inval.l: Updated.
>>> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
>>> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
>>> tests.
>>> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
>>
>> The new insn forms you add don't make clear what exactly you're after:
>> The %cs operands make them invalid no matter what immediate is used. I
>> don't mind you adding "bogus register" forms, but that's not related
>> to the purpose of the patch. $y, otoh, looks like an entirely valid
>> operand to me: y can be an absolute symbol defined elsewhere. So what
>> exactly is it that you're trying to (a) prevent and (b) test?
>>
>> As an aside, adding further insn forms to the 32-bit testcase looks
>> pretty meaningless to me. The forms with operands aren't supported
>> outside of 64-bit mode anyway, no matter what exactly the operands
>> are.
> 
> Assembler crashed at:
> 
>       if (is_cpu(current_templates.start, CpuUSER_MSR)
>           || t->mnem_off == MN_rdmsr
>           || t->mnem_off == MN_wrmsrns)
>         {
>           for (j = 0; j < i.imm_operands; j++)
>             i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>         }
> 
> My patch prevents segfault.  The new testcases caused segfault in
> both 32-bit and 64-bit modes.

I understand that (from Alan's report), albeit I still don't quite
understand why. Yes, we're still ahead of match_template(), but if
i.imm_operands is non-zero the first so many operands should be
immediates, shouldn't they? If not, I would think the operand
swapping immediately ahead of the if() you alter may need adjustment
instead. (Thinking of it, other insns covered there may similarly be
affected, which would even more so call for a correction there.)

Plus the testcase additions don't make clear which operand it is
that the problem was with, as for both operands you use a form which
isn't otherwise tested. I'm (now) guessing it's the %cs one, but it
could as well be the $f. I'm (now) further guessing that you may not
even need to resort to a segment register. A GPR may do as well,
making the testcase less special.

Jan
  

On Wed, Apr 8, 2026 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.04.2026 09:27, H.J. Lu wrote:
> > On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 08.04.2026 02:22, H.J. Lu wrote:
> >>> Verify immediate operand before processing immediate for rdmsr and
> >>> wrmsrns.
> >>>
> >>> PR gas/34028
> >>> * config/tc-i386.c (i386_assemble): Verify immediate operand
> >>> before processing immediate for rdmsr and wrmsrns.
> >>> * testsuite/gas/i386/msr_imm-inval.l: Updated.
> >>> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
> >>> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
> >>> tests.
> >>> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
> >>
> >> The new insn forms you add don't make clear what exactly you're after:
> >> The %cs operands make them invalid no matter what immediate is used. I
> >> don't mind you adding "bogus register" forms, but that's not related
> >> to the purpose of the patch. $y, otoh, looks like an entirely valid
> >> operand to me: y can be an absolute symbol defined elsewhere. So what
> >> exactly is it that you're trying to (a) prevent and (b) test?
> >>
> >> As an aside, adding further insn forms to the 32-bit testcase looks
> >> pretty meaningless to me. The forms with operands aren't supported
> >> outside of 64-bit mode anyway, no matter what exactly the operands
> >> are.
> >
> > Assembler crashed at:
> >
> >       if (is_cpu(current_templates.start, CpuUSER_MSR)
> >           || t->mnem_off == MN_rdmsr
> >           || t->mnem_off == MN_wrmsrns)
> >         {
> >           for (j = 0; j < i.imm_operands; j++)
> >             i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >         }
> >
> > My patch prevents segfault.  The new testcases caused segfault in
> > both 32-bit and 64-bit modes.
>
> I understand that (from Alan's report), albeit I still don't quite
> understand why. Yes, we're still ahead of match_template(), but if
> i.imm_operands is non-zero the first so many operands should be
> immediates, shouldn't they? If not, I would think the operand
> swapping immediately ahead of the if() you alter may need adjustment
> instead. (Thinking of it, other insns covered there may similarly be
> affected, which would even more so call for a correction there.)
>
> Plus the testcase additions don't make clear which operand it is

The problem is %cs:.

> that the problem was with, as for both operands you use a form which
> isn't otherwise tested. I'm (now) guessing it's the %cs one, but it
> could as well be the $f. I'm (now) further guessing that you may not

You guessed wrong:

[hjl@gnu-tgl-3 tmp]$ cat x.s
.text
foo:
wrmsrns $y,%rax
wrmsrns $y,%eax
[hjl@gnu-tgl-3 tmp]$ gcc -c x.s
x.s: Assembler messages:
x.s:3: Error: operand type mismatch for `wrmsrns'
x.s:4: Error: operand type mismatch for `wrmsrns'
[hjl@gnu-tgl-3 tmp]$

> even need to resort to a segment register. A GPR may do as well,
> making the testcase less special.
>
> Jan
  

On 08.04.2026 11:55, H.J. Lu wrote:
> On Wed, Apr 8, 2026 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.04.2026 09:27, H.J. Lu wrote:
>>> On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 08.04.2026 02:22, H.J. Lu wrote:
>>>>> Verify immediate operand before processing immediate for rdmsr and
>>>>> wrmsrns.
>>>>>
>>>>> PR gas/34028
>>>>> * config/tc-i386.c (i386_assemble): Verify immediate operand
>>>>> before processing immediate for rdmsr and wrmsrns.
>>>>> * testsuite/gas/i386/msr_imm-inval.l: Updated.
>>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
>>>>> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
>>>>> tests.
>>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
>>>>
>>>> The new insn forms you add don't make clear what exactly you're after:
>>>> The %cs operands make them invalid no matter what immediate is used. I
>>>> don't mind you adding "bogus register" forms, but that's not related
>>>> to the purpose of the patch. $y, otoh, looks like an entirely valid
>>>> operand to me: y can be an absolute symbol defined elsewhere. So what
>>>> exactly is it that you're trying to (a) prevent and (b) test?
>>>>
>>>> As an aside, adding further insn forms to the 32-bit testcase looks
>>>> pretty meaningless to me. The forms with operands aren't supported
>>>> outside of 64-bit mode anyway, no matter what exactly the operands
>>>> are.
>>>
>>> Assembler crashed at:
>>>
>>>       if (is_cpu(current_templates.start, CpuUSER_MSR)
>>>           || t->mnem_off == MN_rdmsr
>>>           || t->mnem_off == MN_wrmsrns)
>>>         {
>>>           for (j = 0; j < i.imm_operands; j++)
>>>             i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
>>>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>         }
>>>
>>> My patch prevents segfault.  The new testcases caused segfault in
>>> both 32-bit and 64-bit modes.
>>
>> I understand that (from Alan's report), albeit I still don't quite
>> understand why. Yes, we're still ahead of match_template(), but if
>> i.imm_operands is non-zero the first so many operands should be
>> immediates, shouldn't they? If not, I would think the operand
>> swapping immediately ahead of the if() you alter may need adjustment
>> instead. (Thinking of it, other insns covered there may similarly be
>> affected, which would even more so call for a correction there.)
>>
>> Plus the testcase additions don't make clear which operand it is
> 
> The problem is %cs:.
> 
>> that the problem was with, as for both operands you use a form which
>> isn't otherwise tested. I'm (now) guessing it's the %cs one, but it
>> could as well be the $f. I'm (now) further guessing that you may not
> 
> You guessed wrong:

Well, okay, yet ...

> [hjl@gnu-tgl-3 tmp]$ cat x.s
> .text
> foo:
> wrmsrns $y,%rax
> wrmsrns $y,%eax
> [hjl@gnu-tgl-3 tmp]$ gcc -c x.s
> x.s: Assembler messages:
> x.s:3: Error: operand type mismatch for `wrmsrns'
> x.s:4: Error: operand type mismatch for `wrmsrns'

... can you also explain the difference in behavior for segment register
vs GPR use? Does it maybe merely happen to crash for the former but not
the latter in the specific build you work with? Where what fix wants
making depends on properly understanding what is actually going wrong.
Otherwise we just stack hacks on one another without really sorting the
root cause. If you don't want to take the time, I can, but only later in
the week.

Jan
  

On Wed, Apr 8, 2026 at 6:08 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.04.2026 11:55, H.J. Lu wrote:
> > On Wed, Apr 8, 2026 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 08.04.2026 09:27, H.J. Lu wrote:
> >>> On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 08.04.2026 02:22, H.J. Lu wrote:
> >>>>> Verify immediate operand before processing immediate for rdmsr and
> >>>>> wrmsrns.
> >>>>>
> >>>>> PR gas/34028
> >>>>> * config/tc-i386.c (i386_assemble): Verify immediate operand
> >>>>> before processing immediate for rdmsr and wrmsrns.
> >>>>> * testsuite/gas/i386/msr_imm-inval.l: Updated.
> >>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
> >>>>> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
> >>>>> tests.
> >>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
> >>>>
> >>>> The new insn forms you add don't make clear what exactly you're after:
> >>>> The %cs operands make them invalid no matter what immediate is used. I
> >>>> don't mind you adding "bogus register" forms, but that's not related
> >>>> to the purpose of the patch. $y, otoh, looks like an entirely valid
> >>>> operand to me: y can be an absolute symbol defined elsewhere. So what
> >>>> exactly is it that you're trying to (a) prevent and (b) test?
> >>>>
> >>>> As an aside, adding further insn forms to the 32-bit testcase looks
> >>>> pretty meaningless to me. The forms with operands aren't supported
> >>>> outside of 64-bit mode anyway, no matter what exactly the operands
> >>>> are.
> >>>
> >>> Assembler crashed at:
> >>>
> >>>       if (is_cpu(current_templates.start, CpuUSER_MSR)
> >>>           || t->mnem_off == MN_rdmsr
> >>>           || t->mnem_off == MN_wrmsrns)
> >>>         {
> >>>           for (j = 0; j < i.imm_operands; j++)
> >>>             i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
> >>>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>         }
> >>>
> >>> My patch prevents segfault.  The new testcases caused segfault in
> >>> both 32-bit and 64-bit modes.
> >>
> >> I understand that (from Alan's report), albeit I still don't quite
> >> understand why. Yes, we're still ahead of match_template(), but if
> >> i.imm_operands is non-zero the first so many operands should be
> >> immediates, shouldn't they? If not, I would think the operand
> >> swapping immediately ahead of the if() you alter may need adjustment
> >> instead. (Thinking of it, other insns covered there may similarly be
> >> affected, which would even more so call for a correction there.)
> >>
> >> Plus the testcase additions don't make clear which operand it is
> >
> > The problem is %cs:.
> >
> >> that the problem was with, as for both operands you use a form which
> >> isn't otherwise tested. I'm (now) guessing it's the %cs one, but it
> >> could as well be the $f. I'm (now) further guessing that you may not
> >
> > You guessed wrong:
>
> Well, okay, yet ...
>
> > [hjl@gnu-tgl-3 tmp]$ cat x.s
> > .text
> > foo:
> > wrmsrns $y,%rax
> > wrmsrns $y,%eax
> > [hjl@gnu-tgl-3 tmp]$ gcc -c x.s
> > x.s: Assembler messages:
> > x.s:3: Error: operand type mismatch for `wrmsrns'
> > x.s:4: Error: operand type mismatch for `wrmsrns'
>
> ... can you also explain the difference in behavior for segment register
> vs GPR use? Does it maybe merely happen to crash for the former but not

They are different things.  They aren't interchangeable in most cases.

> the latter in the specific build you work with? Where what fix wants
> making depends on properly understanding what is actually going wrong.
> Otherwise we just stack hacks on one another without really sorting the
> root cause. If you don't want to take the time, I can, but only later in
> the week.
>

Please go ahead.  BTW, can you assign

https://sourceware.org/bugzilla/show_bug.cgi?id=34028

to yourself?

Thanks.
  

On 08.04.2026 11:55, H.J. Lu wrote:
> On Wed, Apr 8, 2026 at 5:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.04.2026 09:27, H.J. Lu wrote:
>>> On Wed, Apr 8, 2026 at 2:33 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 08.04.2026 02:22, H.J. Lu wrote:
>>>>> Verify immediate operand before processing immediate for rdmsr and
>>>>> wrmsrns.
>>>>>
>>>>> PR gas/34028
>>>>> * config/tc-i386.c (i386_assemble): Verify immediate operand
>>>>> before processing immediate for rdmsr and wrmsrns.
>>>>> * testsuite/gas/i386/msr_imm-inval.l: Updated.
>>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.l: Likewise.
>>>>> * testsuite/gas/i386/msr_imm-inval.s: Add new rdmsr and wrmsrns
>>>>> tests.
>>>>> * testsuite/gas/i386/x86-64-msr_imm-inval.s: Likewise.
>>>>
>>>> The new insn forms you add don't make clear what exactly you're after:
>>>> The %cs operands make them invalid no matter what immediate is used. I
>>>> don't mind you adding "bogus register" forms, but that's not related
>>>> to the purpose of the patch. $y, otoh, looks like an entirely valid
>>>> operand to me: y can be an absolute symbol defined elsewhere. So what
>>>> exactly is it that you're trying to (a) prevent and (b) test?
>>>>
>>>> As an aside, adding further insn forms to the 32-bit testcase looks
>>>> pretty meaningless to me. The forms with operands aren't supported
>>>> outside of 64-bit mode anyway, no matter what exactly the operands
>>>> are.
>>>
>>> Assembler crashed at:
>>>
>>>       if (is_cpu(current_templates.start, CpuUSER_MSR)
>>>           || t->mnem_off == MN_rdmsr
>>>           || t->mnem_off == MN_wrmsrns)
>>>         {
>>>           for (j = 0; j < i.imm_operands; j++)
>>>             i.types[j] = smallest_imm_type (i.op[j].imms->X_add_number);
>>>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>         }
>>>
>>> My patch prevents segfault.  The new testcases caused segfault in
>>> both 32-bit and 64-bit modes.
>>
>> I understand that (from Alan's report), albeit I still don't quite
>> understand why. Yes, we're still ahead of match_template(), but if
>> i.imm_operands is non-zero the first so many operands should be
>> immediates, shouldn't they? If not, I would think the operand
>> swapping immediately ahead of the if() you alter may need adjustment
>> instead. (Thinking of it, other insns covered there may similarly be
>> affected, which would even more so call for a correction there.)
>>
>> Plus the testcase additions don't make clear which operand it is
> 
> The problem is %cs:.

Thinking of it, is the above a legal operand spelling in the first place?
For the testsuite adjustments I'm going to use something more "normal",
as the property needed really is a memory operand without displacement.
Yet independently I wonder whether we shouldn't reject segment overrides
without following memory reference as such, not by hitting the general
"operand type mismatch" diagnostic. (In Intel syntax we - the expression
parser, that is - at least warn about such.)

Actually, we try to, but that fails (also for an operand consisting of
just "*"): starts_memory_operand() returns true for a nul character. It
pretty clearly shouldn't, I'm inclined to say. I guess I will need to
make a 2nd patch.

Jan