Are uint32 values must be zero-extended in v8?

[Zhao Jiazhong]

Hello everyone,

AFAIK, MIPS64 and RV64 don't have 32-bit registers and according to their calling convention, they would sign-extend all 32-bit values when they are stored in 64-bit registers, both signed and unsigned, see: A related question in stackoverflow.

But in v8 it seems the uint32 values are zero-extended? I tried to sign-extend uint32 values,  on mips64, which could elide some instructions, and only two more tests failed:

- cctest/test-run-load-store/RunLoadStoreZeroExtend64

- cctest/test-run-load-store/RunUnalignedLoadStoreZeroExtend64

Just like their names suggest, they both may load some values and check whether they are zero-extended.

So I wonder could uint32  values be sign-extended in v8 on MIPS64? Any suggestion would be appreciated, thanks!

Thanks,

Zhao Jiazhong

[Andreas Haas]

Hello,

As far as I know V8 does not rely on zero-extension. Typically there would be `ChangeUint32ToUint64` node in the TurboFan graph, which can then get eliminated away in the platform specific code, see e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/x64/instruction-selector-x64.cc;l=1536;drc=ae1eee10fab6ea738b8289f9dc4144e38425a463.

That being said, sometimes such nodes are missing, which can lead to security bugs. That's why there is lots of code checking for zero extension, see e.g. https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/x64/code-generator-x64.cc;l=1898;drc=6bd44dfe57d8d9673c34eda1ae27d66a7f5ec637.

It could be that we should actually delete these tests, maybe they are out-dated. I have to check that.

Cheers, Andreas

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/a9163642-7e38-410a-8476-8bb4a5c545d0n%40googlegroups.com.

[Jakob Kummerow]

There isn't really such a thing as "sign-extending a uint", as "uint" means "unsigned int". Zero-extension is the only extension that makes sense for uint32.

 I think we zero-extend most 32-bit values in V8, however there are a few cases where we must explicitly sign-extend: specifically when a (signed) int32 will be added to a 64-bit value, such as for address+offset computations.

We do rely on 32-bit values being zero-extended in some places, so it's important to keep those tests working.

(There may be room for significant cleanup/redesign there; but our historical experience is that bugs very easily slip in there, so I'd rather not change any assumptions unless there's a really strong reason to do so.)

On Mon, Jul 26, 2021 at 2:31 PM Zhao Jiazhong <zhaojia...@loongson.cn> wrote:

--

[Zhao Jiazhong]

Hello Andreas and Jakob,

Thanks for all your onformation, but I still have some questions.

> That being said, sometimes such nodes are missing, which can lead to security bugs. That's why there is lots of code checking for zero extension, see e.g.  https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/x64/code-generator-x64.cc;l=1898;drc=6bd44dfe57d8d9673c34eda1ae27d66a7f5ec637.

Is keep uint32 values "sign-extended"(extend bit 31 into bits 63 through 32) or `ChangeUint32ToUint64` without zero-extension that may lead to security bugs? If it's the former, could you give more details? Thanks!

> There isn't really such a thing as "sign-extending a uint", as "uint" means "unsigned int". Zero-extension is the only extension that makes sense for uint32.

Sorry for my lax use of terminology. "sign-extending a uint" means extending the 32-bit value's bit 31 into bits 63 through 32, it's the way MIPS64 and probably RV64I handle both signed and unsigned 32-bit values.

Since MIPS64 don't have real unsigned 32-bit instructions, AFAIK, most of it's 32-bit instructions like add/addu/sub/subu/div/mul require the input values are "sign-extended" and the output wil be sign-extended too.

> I think we zero-extend most 32-bit values in V8, however there are a few cases where we must explicitly sign-extend: specifically when a (signed) int32 will be added to a 64-bit value, such as for address+offset computations.

> We do rely on 32-bit values being zero-extended in some places, so it's important to keep those tests working.

Will 32-bit values be added to 64-bit values without promotion to 64-bit? I think if the operations are all between 32-bit values, then the "sign-exntension" won't cause problems?

Besides, I thought only MIPS64 and RV64 care about this issue, X64 and ARM64 have full 32-bit registers support, why do they still rely on the zero-extension? Thanks!

Thanks,

Zhao Jiazhong

[Jakob Kummerow]

> That being said, sometimes such nodes are missing, which can lead to security bugs. That's why there is lots of code checking for zero extension, see e.g.  https://source.chromium.org/chromium/chromium/src/+/main:v8/src/compiler/backend/x64/code-generator-x64.cc;l=1898;drc=6bd44dfe57d8d9673c34eda1ae27d66a7f5ec637.

Is keep uint32 values "sign-extended"(extend bit 31 into bits 63 through 32) or `ChangeUint32ToUint64` without zero-extension that may lead to security bugs? If it's the former, could you give more details?

Probably both, though the former seems harder to trigger (as uint32 values with the top bit 1 are probably rare, and if the top bit is 0 anyway then "sign-extension" of course does the same as zero-extension). Wasm memories and/or TypedArrays with >2GB size might be ways to flush out issues.

 

> I think we zero-extend most 32-bit values in V8, however there are a few cases where we must explicitly sign-extend: specifically when a (signed) int32 will be added to a 64-bit value, such as for address+offset computations.

> We do rely on 32-bit values being zero-extended in some places, so it's important to keep those tests working.

Will 32-bit values be added to 64-bit values without promotion to 64-bit? I think if the operations are all between 32-bit values, then the "sign-exntension" won't cause problems?

Besides, I thought only MIPS64 and RV64 care about this issue, X64 and ARM64 have full 32-bit registers support, why do they still rely on the zero-extension?

We commonly add 32-bit offsets to 64-bit object start addresses. There should be an explicit 64-bit promotion in every case, though we've seen bugs where that was missing. Since x86 hardware interprets offsets as signed values, doing a sign-extension of a value that's meant to be a uint (and hence positive) would change the result of the computation. Since we're talking about memory addresses, incorrect computations are crashes at best and security bugs at worst. (In fact, on x86 we've (also?) encountered the opposite issue: negative offsets do show up in some cases (e.g. HeapObject::kMapOffset, or when iterating/indexing backwards for some reason), and those have to be sign-extended.)

My key takeaways are:

(1) zero- vs. sign-extension matters, especially when 32-bit offsets and 64-bit base addresses are concerned

(2) it's hard to get this right, because there are so many code paths where some variation of this happens. So it makes sense to be super diligent, e.g. by adding extra checks (behind #ifdef DEBUG and/or FLAG_debug_code) and fuzzer tests.

[Zhao Jiazhong]

Thanks for your reply! I will study it further and do more checks and tests.