Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EX and EXRL can produce spurious PIC 3 (S0C3) with a crafted test case #415

Closed
TonyH-Git opened this issue Aug 26, 2021 · 5 comments
Closed
Assignees
Labels
BUG The issue describes likely incorrect product functionality that likely needs corrected.

Comments

@TonyH-Git
Copy link

TonyH-Git commented Aug 26, 2021

This test was run on Juergen's system wotho4.ethz.ch at reported Hercules Version : 4.3.9999.10200-SDL-g208e766-modified. Looking at the current code on Github, I see no updates that would have fixed this.

Execute of an Execute is defined to produce a program interruption 3. The Hercules implementation has a bug that - in a contrived case - produces such an exception even when the target is not another execute instruction.

Here is that case:

l 1r
  1R  00000005                      R1 contains 5
 TEST

l 15r% i l(22)                      R15 is our code base
 0B8BA8.    EX    1,16(0,15)        Mainline EX instruction targeting
                                    an EXRL at +10 hex = 16 dec
 0B8BAC.    BCR   0,0               Some filler NOPRs to aid debugging...
 0B8BAE.    BCR   0,0
 0B8BB0.    BCR   0,0
 0B8BB2.    BCR   0,0
 0B8BB4.    BCR   0,0
 0B8BB6.    BCR   0,0
 0B8BB8.    DC    X'C600'           This is an EXRL instruction. (TSO TEST
                                    doesn't know how to format it.)
 0B8BBA.    DC    X'0000'
 0B8BBC.    DC    X'0010'
 TEST

go 15r%
 IKJ56641I SYSTEM ABEND CODE 0C3   REASON CODE 003
 TEST

This happens because the test for Execute of Execute is made before the low byte of the register (in this case R1) is OR'd with the second byte of the target of the Execute.

In the case of EX/EXRL of EX, this is fine. In the case of EX/EXRL of EXRL however, the second byte is part of the opcode, and so after the OR, if the low byte of the register is not 0, it won't be an EXRL instruction anymore. In the above example, R1 contains 5, which turns the coded EXRL (C6x0) into a CHRL (C6x5) (Compare Halfword Relative Long), which is a completely valid instruction to be EXecuted.

Obviously this is a bizarre corner case that nobody in the Real World is going to encounter. But it looks trivial to fix, and on principle it should be done, though certainly with low priority.

I have confirmed that on both Real Iron (zBC12) and a zPDT, the behaviour matches my expectations.

@Juergen-Git
Copy link
Collaborator

Juergen-Git commented Aug 29, 2021

Hi Toni,

Well it looks like yet another day one bug! We've seen quite a few in Hercules recently. Thanks for reporting it. I'll take a look at it. It might be a welcome distraction from the harder to fix issues we're currently having.  ;-)

Cheers.
Jürgen

@Juergen-Git Juergen-Git self-assigned this Aug 29, 2021
@Fish-Git Fish-Git added the BUG The issue describes likely incorrect product functionality that likely needs corrected. label Aug 29, 2021
@TonyH-Git
Copy link
Author

TonyH-Git commented Aug 29, 2021

Some time ago I started thinking about this odd notion of EX changing the target instruction opcode.

I'm sure this has extremely limited legitimate use (well I guess TRAP2 and TRAP4 were intentionally made xxFF opcodes so they can be EXecuted by changing another opcode), but EX was defined very simply the way it is in 1964 when all opcodes were in just the first byte, and when they introduced opcodes that are partly in the second byte - well what else could they do?

I suppose they could have defined EX so that it doesn't do the OR if it would change the opcode, but that would probably add to the decoding overhead. It still has to OR the part of the byte that isn't the opcode (typically a register number).

There is no integrity or security exposure that I can see from this architectural decision, even though one can change from an unpriv to a priv instruction. But of course a bug in an emulator (or Real Iron, I suppose) could lead to badness.

IIRC one of those IBM architecture papers says that EX is treated like a branch instruction that is always predicted to succeed, but that is discovered to always fail (after execution of the target). And of course there needs to be special handling if the EXecuted instruction is itself a branch or an LPSW or the like.

Here's how I can imagine an integrity bug working:  (I'm not at all claiming that there is such a bug in Hercules!)

If some kind of pre-checking for priv vs. non-priv instructions was performed in an I-unit (perhaps as part of a general pre-fetch and instruction setup operation) and the opcode was then changed by EX from non-priv to priv, then we'd have a classic TOCTTOU ("Time-of-check to time-of-use") exposure.

I suppose it would be pretty easy to compile a table of all possible opcodes that can be converted by EX from non-priv to priv, and then try them all on various hardware and emulators. Looks as though the instruction formats with part of the opcode in the second byte are E, IE, RI, RIL, RRD, RRE, RRF, S, SIL, SSF.

@Fish-Git Fish-Git assigned Fish-Git and unassigned Juergen-Git Jul 5, 2023
@Fish-Git Fish-Git added the IN PROGRESS... I'm working on it! (Or someone else is!) label Jul 5, 2023
Fish-Git added a commit that referenced this issue Jul 5, 2023
@Fish-Git
Copy link
Member

Fish-Git commented Jul 5, 2023

Fixed by commit cbc3d75.

Closing.

Tony?  (@TonyH-Git )

If you can (and are willing), -OR- if you want, please pull current 'develop' branch git, rebuild and retest. It should work as expected now.

Thanks.

@Fish-Git Fish-Git closed this as completed Jul 5, 2023
@Fish-Git Fish-Git removed the IN PROGRESS... I'm working on it! (Or someone else is!) label Jul 5, 2023
@wrljet
Copy link
Member

wrljet commented Jul 5, 2023

Fish,

The new test, ex.txt, being of the .txt variety, is not automatically run, correct?
Any reason why or why not for this test?

Bill

@Fish-Git
Copy link
Member

Fish-Git commented Jul 5, 2023

The new test, ex.txt, being of the .txt variety, is not automatically run, correct?

Correct.

Any reason why or why not for this test?

Yes.  :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG The issue describes likely incorrect product functionality that likely needs corrected.
Projects
None yet
Development

No branches or pull requests

4 participants