jasmin-lang / jasmin

Language for high-assurance and high-speed cryptography
MIT License
268 stars 55 forks source link

Incorrect flags update for `IMULr` #524

Closed basavesh closed 1 year ago

basavesh commented 1 year ago

This bug is found due to fuzz-test. (the bug is present in all IMULr variants, though it is easily reproducible for size 16) In some cases, the flags CF and OF are not set properly.

I think the Jasmin semantic is wrong as it looks for overflow (I might be wrong)

Executing Instruction IMULr_16_R12_RSI -> imulw %si, %r12w

Before:

RSI: 9416389359609642987
R12: 13449037565095184505
CF: false
PF: false
ZF: false
SF: false
OF: false

After:

RSI: 9416389359609642987
R12: 13449037565095202323
CF: false
PF: undef
ZF: undef
SF: undef
OF: false

However, in the above case, Hardware sets the CF and OF flags.

These crashes are easily reproducible. Edit: removed irrelevant data CC: @vbgl @bgregoir @cryptojedi @gbarthe

basavesh commented 1 year ago

The same bug exists for IMUL and I believe it will be there in IMULri too as they are all variants of x86 imul instruction.

vbgl commented 1 year ago

Please be concise and to the point. There is way too much irrelevant data in this report.

bgregoir commented 1 year ago

Sorry but I don't understand this bug report. What is the meaning of Before and After ? what is the problem with flag ? If the problem is that some flag are undef, this is not a bug.

vbgl commented 1 year ago

Tentative fix in #528.

basavesh commented 1 year ago

sorry for the confusion. Before is asm state before executing the instruction and after is the result after executing that instruction.

The bug here is, for that input CF and OF should true. However Jasmin has set it to false.

basavesh commented 1 year ago

My dumb fuzzer is happy with the fix. Usually it triggers the bug within couple of seconds and I don't see crashes anymore after the fix.