Cage for ExternalStrings when memory corruption API [v8/v8 : main]
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +0 |
1 comment
PTAL - this is not finished, but sending this out early in case of any high-level feedback. Thanks!
Related details
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 7 comments![]( "Open in Gerrit")
The direction looks good to me
data_ = new Char[length_];You need to distinguish one byte strings from two byte strings here.
auto* cage = reinterpret_cast<v8::internal::Isolate*>(isolate_)Instead of storing the isolate, you can just store the cage itself. It shouldn't change during execution.
Alternatively:
If `SimpleStringResource` lives in the sandbox, it can be corrupted as well. In that case, maybe don't store anything and just get the isolate via `Isolate::Current()`? I believe this should always happen on the main thread with the current isolate properly set up, so it's probably the safer option.
void ExternalStringsCage::Initialize() {I think this initialization is not sufficient as it not guarantees that the allocator doesn't allocate at the end of the cage. I think that after reserving the memory you need to allocate (or "allocate") the buffer part of it so that nothing else can be allocated there.
.reservation_size = kMaxContentsSize + kBufferSize,Shouldn't it suffice to just allocate `kBufferSize`? I assumed you'd use the first half for allocations and the second half for the buffer.
size_t page_count = (buf_size + page_size_ - 1) / page_size_;Why do you need to allocate whole pages?
i_isolate->isolate_group()->external_strings_cage()->reservation_region();What would this return if we didn't initialize the cage? Or are we always guaranteed to have it initialized when we use it here?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 6 comments![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Commit-Queue | +1 |
6 comments
You need to distinguish one byte strings from two byte strings here.
|Char| is already a template parameter, so the array allocation is OK here I think? But I've renamed it to "CharT" to make it more apparent.
auto* cage = reinterpret_cast<v8::internal::Isolate*>(isolate_)Instead of storing the isolate, you can just store the cage itself. It shouldn't change during execution.
Alternatively:
If `SimpleStringResource` lives in the sandbox, it can be corrupted as well. In that case, maybe don't store anything and just get the isolate via `Isolate::Current()`? I believe this should always happen on the main thread with the current isolate properly set up, so it's probably the safer option.
Simplified using IsolateGroup::current(), thanks.
I think this initialization is not sufficient as it not guarantees that the allocator doesn't allocate at the end of the cage. I think that after reserving the memory you need to allocate (or "allocate") the buffer part of it so that nothing else can be allocated there.
Done, totally forgot about this.
Shouldn't it suffice to just allocate `kBufferSize`? I assumed you'd use the first half for allocations and the second half for the buffer.
Renamed the second constant to kGuardRegionSize to clarify the expression here. I.e., it's 4 GB for strings themselves, and 8 GB for the guard region (covering the maximum possible 32-bit shift in a UTF-16 string). Hopefully that makes sense.
size_t page_count = (buf_size + page_size_ - 1) / page_size_;Why do you need to allocate whole pages?
Added a comment explaining it - potentially we could've used PartitionAlloc, but it's not possible at the moment. So there's an obvious memory overhead here, but it's probably not too terrible if we only talk about using this in fuzzers (IIUC the page size is around 4KB).
As written in the doc, we could potentially lift this restriction by switching to PartitionAlloc here, but even if we do I'd propose to do it in a separate page. Also with PA it won't be easy to enforce "external string contents are sealed" in fuzzers, which might be a useful property to check.
i_isolate->isolate_group()->external_strings_cage()->reservation_region();What would this return if we didn't initialize the cage? Or are we always guaranteed to have it initialized when we use it here?
Added a check that'd prevent using reservation_region() before the initialization.
Related details
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 7 comments![]( "Open in Gerrit")
String::WriteToFlat(*string, reinterpret_cast<uint8_t*>(resource->data()),Since you're using whole pages for the strings, can you mark them as read only after copying the string?
We could introduce a method like `SimpleStringResource::Seal()` that does that.
// a redzone and occupy whole pages for a string at the moment.Why do we use a redzone? Is it for catching small OOB reads?
An "attacker" can just "jump" over the redzone to read from the next external string, so I'm not sure what we get from having a redzone. Is a fuzzer more likely to try a small OOB read?
Is it just because you needed padding the remainder of the page? If that's the case then I don't think we should `kRedzoneMinSize` in `ExternalStringsCage::GetAllocSize` because if a string is a whole page then there's no reason to waste another page just a redzone imo.
.reservation_size = kMaxContentsSize + kBufferSize,Maksim IvanovShouldn't it suffice to just allocate `kBufferSize`? I assumed you'd use the first half for allocations and the second half for the buffer.
Renamed the second constant to kGuardRegionSize to clarify the expression here. I.e., it's 4 GB for strings themselves, and 8 GB for the guard region (covering the maximum possible 32-bit shift in a UTF-16 string). Hopefully that makes sense.
Thanks for clarifying. I didn't consider the two-byte-string case.
size_t page_count = (buf_size + page_size_ - 1) / page_size_;Maksim IvanovWhy do you need to allocate whole pages?
Added a comment explaining it - potentially we could've used PartitionAlloc, but it's not possible at the moment. So there's an obvious memory overhead here, but it's probably not too terrible if we only talk about using this in fuzzers (IIUC the page size is around 4KB).
As written in the doc, we could potentially lift this restriction by switching to PartitionAlloc here, but even if we do I'd propose to do it in a separate page. Also with PA it won't be easy to enforce "external string contents are sealed" in fuzzers, which might be a useful property to check.
Acknowledged
// Allocate whole pages as we're relying on BoundedPageAllocator (e.g.,I was wondering why we need to use a BoundedPageAllocator here and not one of our smaller granularity allocators, but I suppose they are too specialized for our regular allocations to work here, right?
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 5 comments![]( "Open in Gerrit")
#endifMaksim Ivanovnit: `#endif // V8_ENABLE_MEMORY_CORRUPTION_API`
Done
String::WriteToFlat(*string, reinterpret_cast<uint8_t*>(resource->data()),Since you're using whole pages for the strings, can you mark them as read only after copying the string?
We could introduce a method like `SimpleStringResource::Seal()` that does that.
Exactly, but I'd propose doing this in a follow-up, to separate the current change (whose purpose is to filter out some reports) from this hardening (which would add a new case when reports are generated).
#endifMaksim Ivanovnit: `#endif // V8_ENABLE_MEMORY_CORRUPTION_API`
Done
// a redzone and occupy whole pages for a string at the moment.Why do we use a redzone? Is it for catching small OOB reads?
An "attacker" can just "jump" over the redzone to read from the next external string, so I'm not sure what we get from having a redzone. Is a fuzzer more likely to try a small OOB read?Is it just because you needed padding the remainder of the page? If that's the case then I don't think we should `kRedzoneMinSize` in `ExternalStringsCage::GetAllocSize` because if a string is a whole page then there's no reason to waste another page just a redzone imo.
OK, removed kRedzoneMinSize for now. My thinking was to mimic the regular ASan behavior which would catch a simple one-byte OOB read via a redzone. Another consideration was that as I round up allocations to page size, without this extra redzone the strings with particular lengths (multiples of the page size) would be treated differently in the fuzzer.
// Allocate whole pages as we're relying on BoundedPageAllocator (e.g.,I was wondering why we need to use a BoundedPageAllocator here and not one of our smaller granularity allocators, but I suppose they are too specialized for our regular allocations to work here, right?
If we agree on sealing (which seems useful), IIUC this requires us to have one-string-per-page, for which a PageAllocator is sufficient. Otherwise, PA sounded like the right tool, even though we could look at alternatives (e.g., one of ideas was ZoneAllocator).
Related details
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
Adding more OWNERS as well - PTAL; thanks!
Related details
- Leszek Swirski
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
I think this already is in good shape
data_ = static_cast<char*>(v8::internal::IsolateGroup::current()
->external_strings_cage()
->Allocate(length_));Can we hide this allocate/free pair behind some RAII struct that is owned by the resource?
Alternatively, let's store at least a span internally. I am aware that there's separate void*/size_t getters which makes this moot but at least we leave the internals in okay shape.
#ifdef V8_ENABLE_MEMORY_CORRUPTION_APINow that I see this here as well: I think it would indeed be better if we could encapsulate the alloc/free a bit more and offer something simple for the callsites.
Related details
- Leszek Swirski
- Maksim Ivanov
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 3 comments![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
3 comments
seems reasonable to me
When fuzzing with memory_corruption_api enabled, move all external string resources into a separate cage with a large reserved guardnit: commit message line wrapping
fprintf(stderr, "External strings cage bounds: [%p,%p)\n",left over? you shouldn't print this unconditionally.
Related details
- Maksim Ivanov
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz added 2 comments![]( "Open in Gerrit")
String::WriteToFlat(*string, reinterpret_cast<uint8_t*>(resource->data()),Maksim IvanovSince you're using whole pages for the strings, can you mark them as read only after copying the string?
We could introduce a method like `SimpleStringResource::Seal()` that does that.
Exactly, but I'd propose doing this in a follow-up, to separate the current change (whose purpose is to filter out some reports) from this hardening (which would add a new case when reports are generated).
Acknowledged
// Allocate whole pages as we're relying on BoundedPageAllocator (e.g.,Maksim IvanovI was wondering why we need to use a BoundedPageAllocator here and not one of our smaller granularity allocators, but I suppose they are too specialized for our regular allocations to work here, right?
If we agree on sealing (which seems useful), IIUC this requires us to have one-string-per-page, for which a PageAllocator is sufficient. Otherwise, PA sounded like the right tool, even though we could look at alternatives (e.g., one of ideas was ZoneAllocator).
I agree. If we go forward with sealing, there's likely no need for PA anymore.
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Omer Katz (Gerrit)
Omer Katz voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Maksim Ivanov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 5 comments![]( "Open in Gerrit")
When fuzzing with memory_corruption_api enabled, move all external string resources into a separate cage with a large reserved guardnit: commit message line wrapping
Done
data_ = static_cast<char*>(v8::internal::IsolateGroup::current()
->external_strings_cage()
->Allocate(length_));Can we hide this allocate/free pair behind some RAII struct that is owned by the resource?
Alternatively, let's store at least a span internally. I am aware that there's separate void*/size_t getters which makes this moot but at least we leave the internals in okay shape.
Makes sense. Added a custom allocator class.
I'm trying to use it with std::vector<T> - seems like it's the simplest way to integrate the custom allocator. However I'll need to check performance benchmarks; if it's visible then std::unique_ptr wiht a custom deleter would be an alternative.
Now that I see this here as well: I think it would indeed be better if we could encapsulate the alloc/free a bit more and offer something simple for the callsites.
OK, replied above.
// Allocate whole pages as we're relying on BoundedPageAllocator (e.g.,Maksim IvanovI was wondering why we need to use a BoundedPageAllocator here and not one of our smaller granularity allocators, but I suppose they are too specialized for our regular allocations to work here, right?
Omer KatzIf we agree on sealing (which seems useful), IIUC this requires us to have one-string-per-page, for which a PageAllocator is sufficient. Otherwise, PA sounded like the right tool, even though we could look at alternatives (e.g., one of ideas was ZoneAllocator).
I agree. If we go forward with sealing, there's likely no need for PA anymore.
ok! resolving
fprintf(stderr, "External strings cage bounds: [%p,%p)\n",left over? you shouldn't print this unconditionally.
The code already prints the sandbox boundaries: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/testing.cc;l=968;drc=325c5b532c768c5d5b9c462d48102fdc071134a6 . I thought same might be useful for the new cage, so that we can debug it easier if something goes wrong with the cage. I can remove it if the benefits don't outweigh the bit of extra spam.
Related details
- Leszek Swirski
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
fprintf(stderr, "External strings cage bounds: [%p,%p)\n",Maksim Ivanovleft over? you shouldn't print this unconditionally.
The code already prints the sandbox boundaries: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/testing.cc;l=968;drc=325c5b532c768c5d5b9c462d48102fdc071134a6 . I thought same might be useful for the new cage, so that we can debug it easier if something goes wrong with the cage. I can remove it if the benefits don't outweigh the bit of extra spam.
- Maksim Ivanov
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Maksim Ivanov
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15ac4838b10000
Related details
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/159f8e0b310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job win-11-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17e4939d310000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/141ed2c0b10000
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Auto-Submit | +1 |
2 comments
data_ = static_cast<char*>(v8::internal::IsolateGroup::current()
->external_strings_cage()
->Allocate(length_));Maksim IvanovCan we hide this allocate/free pair behind some RAII struct that is owned by the resource?
Alternatively, let's store at least a span internally. I am aware that there's separate void*/size_t getters which makes this moot but at least we leave the internals in okay shape.
Makes sense. Added a custom allocator class.
I'm trying to use it with std::vector<T> - seems like it's the simplest way to integrate the custom allocator. However I'll need to check performance benchmarks; if it's visible then std::unique_ptr wiht a custom deleter would be an alternative.
No regressions on common benchmarks, so the std::vector-backed string resource solution seems OK.
fprintf(stderr, "External strings cage bounds: [%p,%p)\n",Maksim Ivanovleft over? you shouldn't print this unconditionally.
Leszek SwirskiThe code already prints the sandbox boundaries: https://source.chromium.org/chromium/chromium/src/+/main:v8/src/sandbox/testing.cc;l=968;drc=325c5b532c768c5d5b9c462d48102fdc071134a6 . I thought same might be useful for the new cage, so that we can debug it easier if something goes wrong with the cage. I can remove it if the benefits don't outweigh the bit of extra spam.
Oh then it's fine
- Leszek Swirski
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16c9208b310000
Related details
- Maksim Ivanov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1353ddc0b10000
Maksim Ivanov (Gerrit)
Maksim Ivanov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Maksim Ivanov
| Auto-Submit | +1 |
| Commit-Queue | +2 |
PTAL - this is not finished, but sending this out early in case of any high-level feedback. Thanks!
Proceeding given two LGTMs and that I addressed Michael's comments. Thanks for the reviews.
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maksim Ivanov (Gerrit)
Maksim Ivanov added 1 comment![]( "Open in Gerrit")
Hmm, apparently I lost LGTMs during one of updates. Please re-stamp; thanks!
Related details
- Leszek Swirski
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted![]( "Open in Gerrit")
- Maksim Ivanov
- Michael Lippautz
- Omer Katz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |