Closed Bug 1684902 Opened 3 years ago Closed 3 years ago

Assert that the register field of LUse can represent every register

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: lth, Assigned: lth)

References

Details

(Keywords: sec-low, Whiteboard: [adv-main86+r])

Attachments

(1 file)

On ARM64 with SIMD, we will go beyond the current 6-bit limit, so insert a static_assert that should have been here all along.

Group: core-security

Actually, ARM64 exceeds the current field even without SIMD because the number of FloatRegisters is 64 and the number of integer registers is 32. The field needs to be 7 bits on ARM64 already.

I'm not sure how bad this is because double registers are currently ordered before single registers and I bet Ion currently only allocates double registers on ARM64 (no asm.js or wasm support, so no need for singles except maybe for Math.fround). Thus this may have worked just fine just by accident.

Group: core-security → javascript-core-security

The register field of LUse is six bits wide but on ARM64 we'll need 7
bits already (32 GPRs + two FP classes of 32 FPUs each) and definitely
if we add SIMD (three FP classes). So assert that this field is wide
enough, and expand it for ARM64.

Depends on D100025

If I read the code correctly, Math.fround may turn into MToFloat32, which has type Float32 and thus may be allocated a float destination register. This register requires seven bits of the form 10xxxxx. The 1 bit will overflow the field. It will be written into the next field, which is the USED_AT_START field, flagging the values as use-at-start rather than plain use. Thus the input register may be reused as the output register, incorrectly, if lowering requests plain use. This would pertain to cases where the input is double or float. But in both those cases, lowering already uses useRegisterAtStart. So that should not be an issue.

More interesting is all the other cases where the USED_AT_START bit is overwritten with a zero when a double register is allocated, because it will have the form 01xxxxx and the 0 will clobber USED_AT_START. There are two subcases. In one case, codegen does not care, and will just work, though regalloc may be suboptimal (additional moves may be inserted). In the other case, codegen does care, but (a) we usually have asserts for this and (b) ARM64 is three-address and this case should therefore be very rare. (Basically CSEL, I think.)

Finally, if the LUse datum is put together with a bitwise OR, the second paragraph should not matter because a bitwise OR of x with zero is still x, thus no bug at all in this case.

Ergo I think sec-low is probably appropriate. nbp, wdyt?

Flags: needinfo?(nicolas.b.pierron)

(In reply to Lars T Hansen [:lth] from comment #3)

If I read the code correctly, Math.fround may turn into MToFloat32, which has type Float32 and thus may be allocated a float destination register. This register requires seven bits of the form 10xxxxx. The 1 bit will overflow the field. It will be written into the next field, which is the USED_AT_START field, flagging the values as use-at-start rather than plain use. Thus the input register may be reused as the output register, incorrectly, if lowering requests plain use. This would pertain to cases where the input is double or float. But in both those cases, lowering already uses useRegisterAtStart. So that should not be an issue.

Looking at the code of LUse, it does not seems to contain the type of the register, only the code of a Register or a FloatRegister which is being used. Thus the assertion added in the patch sounds incorrect.

However, the FloatRegister::code() [x86, arm, arm64] function seems to be capable of returning 7 bits on ARM and ARM64, but we only use the 7th bit when ENABLE_WASM_SIMD is enabled.

On ARM64, a Double is written as 00xxxxx, a Single as 01xxxxx and a SIMD128 as 10xxxxx.

So, it seems this bug is restricted to SIMD128 ARM64 registers, and not float 32 (Single), which would unexpectedly set the USED_AT_START register flag, which would allow temporary data to be leaked into this floating point register, before it is used by any other operations.

More interesting is all the other cases where the USED_AT_START bit is overwritten with a zero when a double register is allocated, […]

Finally, if the LUse datum is put together with a bitwise OR, the second paragraph should not matter because a bitwise OR of x with zero is still x, thus no bug at all in this case.

My understanding is that LUse register code is only set at once with an OR operation. Thus, the used-at-start bit would not overwrite any of the kind bits of the FloatRegister. The only update of LUse seems to be for setting the virtual register.

Ergo I think sec-low is probably appropriate. nbp, wdyt?

I agree.

Flags: needinfo?(nicolas.b.pierron)

Hm, ok. I may have confused myself here by assuming, seeing LUse::ANY, that LUse could store an AnyRegister, since AnyRegister.code() uses a wider encoding. But I can't find an instance where that would be the case. I may still add some assertions in the constructors and/or in LUse::set().

That being so, the assertion should not check Registers::Total+FloatRegisters::Total, but max(Registers::Total, FloatRegisters::Total).

On current arm32, code() is six bits because, though the kind is encoded in the code(), it is only a single bit, single=0 and double=1. The vcvt bit of the kind should only be used in the masm/asm and not show up here, but I will try to verify this. Arm32 will never have SIMD support I think so we can ignore that problem, or assert against it, but either way the corrected assertion in LUse should save us.

On current arm64, without SIMD, the code() is six bits. With SIMD it will be seven bits.

On x64, for valid register values it looks like we need only six bits even with SIMD. The reg_ field is five bits wide but comments and other data show that only four bits are needed for valid data, and the shift count for the kind reflects this. Again, Single=0, Double=1, SIMD=2, for a max of two bits for the kind.

This also means current ARM64 code is correct (not having SIMD support at present). I'll tidy up the patch further and try to open up this bug.

Attachment #9195267 - Attachment description: Bug 1684902 - Check width of LUse reg field, fix for ARM64. r?nbp → Bug 1684902 - Check width of LUse reg field. r?nbp
Keywords: sec-low

Jan, can you open this, i think the conclusion is that this is not s-s, there is no bug at present.

Flags: needinfo?(jdemooij)
Group: javascript-core-security
Flags: needinfo?(jdemooij)
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f309346f397c
Check width of LUse reg field. r=nbp
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
Whiteboard: [adv-main86+r]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: