Fix unchecked memory access for files #1022

pvdrz · 2019-10-24T14:31:14Z

This PR takes care of two problems:

It uses Memory::(read|write)_bytes to guarantee that memory accesses are checked (Fixes: Some direct memory accesses lack bounds-checks #1007)
It removes the (get|remove)_handle_and methods which were a little bit cumbersome to use. In particular remove_handle_and, because we were using it to avoid borrowing issues before the Evaluator::memory field was public.

RalfJung · 2019-10-25T18:27:53Z

When opening a PR that fixes an issue, please put Fixes: #xxx into the PR description so that the issue gets closed automatically when the PR gets merged.

src/shims/fs.rs

RalfJung · 2019-11-02T12:45:11Z

src/shims/fs.rs

+
+            if let Ok(c) = result {
+                // Check that we read less than `i64::MAX` bytes.
+                if c > (i64::max_value() as usize) {


That cast will not work out... rather cast them both to u64 for comparison.

But I think this check is really not needed (as you said before I think); we have the call count many bytes which is already sufficiently bounded.

The weird part is that count is a usize but later you make it an i64... why that? What are the relevant types on the interpreted machine?

the signature for read is:

ssize_t read(int fd, void *buf, size_t count);

So, the count argument is an usize but the return type is isize. I have no idea what should happen if someone passes a value larger than isize::max_value() as count.

Edit: the same is true for write.

These are target usize though. So e.g. on a 32bit host with a 64bit target, these cannot be loslessly converted to host usize.

Target usize is u64 in Miri. Ideally we'd use TryInto to convert that into usize, but unfortunately those instances do not exist. So instead I suggest adding helper methods like

fn try_into_usize(i: impl Into<u128>) -> Option<usize> { let i: u128 = i; if i > usize::max_value() as u128 { None } else { Some(i as usize) } } fn try_from_usize<T: TryFrom<u128>>(i: usize) -> Option<T> { T::try_from(u128::from(i)) }

Hm, that will be really confusing with the scalar to_usize though.^^

Maybe try_into/from_host_usize?

OK I'll add them :)

I added the functions and I'm using them in read and write (see 56c5e53). I don't like how it looks but it works. Maybe we can reformulate try_unwrap_io_result somehow.

src/shims/fs.rs

RalfJung · 2019-11-06T11:44:51Z

src/shims/fs.rs

+            match result {
+                Ok(c) => {
+                    let read_bytes = helpers::try_from_host_usize::<i64>(c)
+                        .ok_or_else(|| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?;


So... here we convert to i64 and then, later, we convert to what the target platform really needs. What will happen if that second part is the one that overflows? It's probably a from_int in foreign_items.rs, and that will ICE if the result is too big. So, the error handling isn't really adequate.

But also, do we need this? We know c <= count. What is the largest possible count? You used to_usize, is that right? It seems the type in C is size_t, which would be isize, not usize! So we should know that the buffer is definitely not too big to represent the result.

The only place where we can actually error is if the program does sth too big for the target, but that is already handled above.

Ah no, size_t is unsigned, I was wrong there. But that's silly, what does the original function do if the size fits into a size_t but not an ssize_t?!? I hate C...

@oli-obk any opinions? My conclusion is that the API we are trying to emulate is inherently broken and there is nothing reasonable we can do, so we should do something simple and consistent.^^

We could interpret the count parameter as signed, so if it is "too big" it'll be negative, and then we usize usize::try_from and if that complains we throw an "unsupported big buffer" or so?

Since the docs on read say

Upon successful completion, read() and pread() shall return a non-negative integer indicating the number of bytes actually read. Otherwise, the functions shall return -1 and set errno to indicate the error.

I would say that any value larger than isize::max_value() (for the target isize) should throw an interpreter error. If the target is 64 bit, it can't happen anyway, because we can't have vectors larger than isize::max_value().

Well, these functions don't have to fill the entire buffer. So I assume what happens is, they ~~truncate~~ saturate the input to isize::MAX and will never use more than that.

right, so the usize we get from Rust's Read::read will never be > i64::max_value(), so for 64bit host platforms interpreting 32 bit target platforms, we basically have to give up when such a situation occurs, since it's too late to do anything about it. Everywhere else, it should be fine

If we truncate the buffer size we pass to Read::read we know the return value can't be bigger than that.

Let me see if IIUC. So that basically means being sure that the count parameter doesn't exceeds isize::max_value() so that when we create the intermediate buffer for reading/writing, it isn't larger than isize::max_value() and we can return a proper value at the end. Sounds good to you?

I think I figured it out for read. I'm not sure if write is OK tho.

src/shims/fs.rs

src/helpers.rs

bors · 2019-11-08T21:43:53Z

☔ The latest upstream changes (presumably #1044) made this pull request unmergeable. Please resolve the merge conflicts.

src/shims/fs.rs

pvdrz · 2019-11-09T14:16:49Z

I'm capping count at the beginning of both functions and try_from(count).unwrap()ing everything.

src/shims/fs.rs

RalfJung · 2019-11-13T22:56:53Z

Thanks!

@bors r+

bors · 2019-11-13T22:56:54Z

📌 Commit 4baef71 has been approved by RalfJung

RalfJung · 2019-11-13T22:57:42Z

src/shims/fs.rs

        if count == 0 {
            return Ok(0);
        }


Note to self: try removing this, now that we don't force_ptr any more.

bors · 2019-11-13T22:58:28Z

⌛ Testing commit 4baef71 with merge 09b0a8a...

@RalfJung

Fix unchecked memory access for files This PR takes care of two problems: - It uses `Memory::(read|write)_bytes` to guarantee that memory accesses are checked (Fixes: #1007) - It removes the `(get|remove)_handle_and` methods which were a little bit cumbersome to use. In particular `remove_handle_and`, because we were using it to avoid borrowing issues before the `Evaluator::memory` field was public. @RalfJung @oli-obk

bors · 2019-11-13T23:19:20Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 09b0a8a to master...

Clean file handling functions

260b463

pvdrz force-pushed the fix-fd-access branch from e2f9c12 to 260b463 Compare October 24, 2019 14:35

pvdrz changed the title ~~Fix unchecked memory access for files.~~ Fix unchecked memory access for files Oct 25, 2019

RalfJung reviewed Oct 25, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 25, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 25, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

pvdrz added 3 commits October 26, 2019 08:54

Drop files explicitly when closing them

d7967f6

Simplify read logic

122549f

Check for usize to i64 overflows

06ef77b

RalfJung reviewed Nov 2, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 2, 2019

View reviewed changes

pvdrz added 2 commits November 3, 2019 10:04

Fix casts for count check

d0b4407

Handle host's usize correctly

56c5e53

RalfJung reviewed Nov 4, 2019

View reviewed changes

src/shims/fs.rs Show resolved Hide resolved

RalfJung reviewed Nov 4, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 4, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Avoid using as cast

f910ea1

RalfJung reviewed Nov 6, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 6, 2019

View reviewed changes

src/helpers.rs Outdated Show resolved Hide resolved

Use TryFrom instead

4bbaa72

Fix merge conflicts

10b9373

RalfJung reviewed Nov 9, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 9, 2019

View reviewed changes

src/shims/fs.rs Show resolved Hide resolved

RalfJung reviewed Nov 9, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Cap count

6e37f2f

RalfJung reviewed Nov 11, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 11, 2019

View reviewed changes

src/shims/fs.rs Show resolved Hide resolved

RalfJung reviewed Nov 11, 2019

View reviewed changes

src/shims/fs.rs Show resolved Hide resolved

RalfJung reviewed Nov 11, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Cap count twice

edd0157

RalfJung reviewed Nov 13, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

RalfJung reviewed Nov 13, 2019

View reviewed changes

src/shims/fs.rs Outdated Show resolved Hide resolved

Fix maximum isize value for target

4baef71

RalfJung reviewed Nov 13, 2019

View reviewed changes

bors merged commit 4baef71 into rust-lang:master Nov 13, 2019

Fix unchecked memory access for files #1022

Fix unchecked memory access for files #1022

Uh oh!

Conversation

pvdrz commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung commented Oct 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvdrz Nov 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Nov 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pvdrz commented Nov 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung commented Nov 13, 2019

Uh oh!

bors commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 13, 2019

Uh oh!

bors commented Nov 13, 2019

Uh oh!

Uh oh!

pvdrz commented Oct 24, 2019 •

edited

Loading

pvdrz Nov 3, 2019 •

edited

Loading

RalfJung Nov 6, 2019 •

edited

Loading