[PATCH] D87258: [WebAssembly, LowerTypeTests] Fix control-flow integrity support

[Dominic Chen via Phabricator via llvm-commits]

ddcc added a comment.

Currently, I am seeing some false positive CFI failures that only occur with WebAssembly and not native, so I still need to look into what's causing that.

In D87258#2261903 <https://reviews.llvm.org/D87258#2261903>, @sbc100 wrote:

> So is the idea that the table indexes would be reserved also by the linker?
>
> Right now wasm-ld completely ignores the table elements in the object files and generates contiguous table entries when if finds TABLE_INDEX relocations.

Ah, so under LTO, the output from WasmObjectWriter is linked by lld with libc, etc, which writes the final output? I was hoping to avoid changing the object format to explicitly assign table element indices, since that would involve amending the specification, by just emitting the table elements in the correct order. Would it be sufficient to have `WasmObjectWriter::recordRelocation` also ensure that TABLE_INDEX relocations are inserted into `CodeRelocations` in the correct assigned order? Right now I only push them to the front of `CodeRelocations` to avoid conflicts when assigning indices with subsequent elements.

In D87258#2262098 <https://reviews.llvm.org/D87258#2262098>, @dschuff wrote:

> Thanks @ddcc for the summary. Given that, I do think it makes sense to fix the current implementation before worrying about multiple tables.

Sounds good, I don't think I have the time right now for the full multiple table design.

In D87258#2262100 <https://reviews.llvm.org/D87258#2262100>, @dschuff wrote:

> (And yes, you are right that using metadata for this isn't strictly correct since metadata can be dropped. And I think you are also correct that we haven't added multiple table support for MC or the instruction definitions either).

One option would be to mutate the function type so that it is in a separate address space, instead of using the !wasm.index metadata. But this may cause problems if we don't clean up all associated function pointers to perform additional address space casts where necessary. Another options could be using function prefix data? Otherwise, I don't see any suitable function attributes that we could reuse, but I also haven't used either of the these options before, so I don't know what would be best.



================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:548
+    // subsequent conflict if an index has already been assigned
+    if (!Rec.Symbol->hasTableIndex())
+      CodeRelocations.push_back(Rec);
----------------
sbc100 wrote:
> Would the read better if you flipped the if/else and remove the negation?
Ok, will do.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:622
     // Ignore overflow. LLVM allows address arithmetic to silently wrap.
-    return Segment.Offset + Ref.Offset + RelEntry.Addend;
+    return Segment.Offset + BaseRef.Offset + SymRef.Offset + RelEntry.Addend;
   }
----------------
sbc100 wrote:
> ddcc wrote:
> > Not sure if this is entirely correct, please take a look. My understanding is that D79462 introduced a bug where it fails to add the offset from a relative symbol. Without these changes, I get the following error from the lld side:
> > 
> > `wasm-ld: warning: unexpected existing value for R_WASM_MEMORY_ADDR_SLEB: existing=8420 expected=8452`
> > 
> > After some debugging, it appears that this is a relative symbol, where the +32 offset gets missed since only the base is examined.
> > 
> > ```
> > RelEntry.Symbol->getVariableValue(false): .L__unnamed_1+32
> > symbol: Name=.L_ZTV3Foo.0, Kind=WASM_SYMBOL_TYPE_DATA, Flags=2, Segment=62, Offset=32, Size=20
> > WasmObjectWriter::writeObject: Ref = {Segment = 62, Offset = 32, Size = 20}
> > WasmObjectWriter::getProvisionalValue: Ref = {Segment = 62, Offset = 0, Size = 52}
> > ```
> This looks correct to me.   Is the reason we have not been seeing because we have `-fdata-sections` on by default so all symbols are in their own section?
> 
> I would hope we could land this separately with its own test(s).  Do you have a way to trigger this bug?
I'll take a look at extracting this part out and writing a testcase for it. Currently, it only occurs when I compile a local test application with CFI enabled using emsdk. I'm not sure where it's happening, but either Clang or LowerTypeTests is inserting references to a function inside the vtable for the Foo class.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87258/new/

https://reviews.llvm.org/D87258