Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
921353002:
sandbox: Fix Win64 porting issue by using SIZE_T instead of ULONG (Closed)
|
Created:
5 years, 10 months ago by Reid Kleckner Modified:
5 years, 10 months ago CC:
chromium-reviews, wfh+watch_chromium.org, rickyz+watch_chromium.org, caitkp+watch_chromium.org, Mark Seaborn Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsandbox: Fix some nt_internals.h function prototypes for Win64
NtQuerySection should use ULONG instead of SIZE_T as it is dealing with PE section sizes.
NtQueryVirtualMemory should use SIZE_T instead of ULONG as it is dealing with memory sizes.
DynamoRIO, another project that consumes these native APIs, uses the prototypes introduced by this change.
R=robertshield@chromium.org,rvargas@chromium.org
BUG=458690
Committed: https://crrev.com/4ede4e174a51d08262edfb2e2c05845504bd9b1a
Cr-Commit-Position: refs/heads/master@{#317177}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Tweak typedefs #Patch Set 3 : upload again #
Messages
Total messages: 33 (5 generated)
thakis@chromium.org changed reviewers: + thakis@chromium.org
I compared the other PSIZE_Ts in the header file. There are other differences. We should probably fix those too? https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:253: IN SIZE_T SectionInformationLength, this is ULONG in dynamorio https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:254: OUT PSIZE_T ReturnLength OPTIONAL); this is PULONG in dynamorio https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:427: IN OUT PSIZE_T RegionSize, This is a PULONG in the dynamorio code https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:459: IN OUT PSIZE_T ProtectSize, This is a PULONG in dynamorio
wfh@chromium.org changed reviewers: + wfh@chromium.org
notes from the peanut gallery https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:253: IN SIZE_T SectionInformationLength, sources differ on this - some say ULONG and some say SIZE_T - on balance I agree this should be ULONG. https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:254: OUT PSIZE_T ReturnLength OPTIONAL); I agree this should be PULONG https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:427: IN OUT PSIZE_T RegionSize, I think this should remain PSIZE_T https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:459: IN OUT PSIZE_T ProtectSize, I think this should remain PSIZE_T
rvargas@chromium.org changed reviewers: + rvargas@chromium.org
https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); We run elf on x64, don't we?. Which means that if this is wrong, GetBackingModuleFilePath should be failing pretty bad.
https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); On 2015/02/17 20:47:30, rvargas wrote: > We run elf on x64, don't we?. Yep. > Which means that if this is wrong, > GetBackingModuleFilePath should be failing pretty bad. Possibly. It could also accidentally work / silently fail most of the time depending on what got passed. We have a very small number of crashes around this code, mostly on x86 so it doesn't appear to be a case of an unnoticed kaboom. Will look tomorrow to see if we're not getting the expected stats on x64.
Tweak typedefs
I think in general, ULONGs are used to describe sizes of PE/COFF constructs, because those are limited to 2GB anyway. SIZE_T is used for virtual memory sizes, since those can be a lot bigger. I made changes along those lines. PTAL https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:253: IN SIZE_T SectionInformationLength, On 2015/02/17 03:05:46, Will Harris wrote: > sources differ on this - some say ULONG and some say SIZE_T - on balance I agree > this should be ULONG. Done. https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:254: OUT PSIZE_T ReturnLength OPTIONAL); On 2015/02/17 03:05:46, Will Harris wrote: > I agree this should be PULONG Done. https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:427: IN OUT PSIZE_T RegionSize, On 2015/02/17 03:05:46, Will Harris wrote: > I think this should remain PSIZE_T Left as PSIZE_T https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); On 2015/02/17 23:54:43, robertshield wrote: > On 2015/02/17 20:47:30, rvargas wrote: > > Which means that if this is wrong, > > GetBackingModuleFilePath should be failing pretty bad. > > Possibly. It could also accidentally work / silently fail most of the time > depending on what got passed. We have a very small number of crashes around this > code, mostly on x86 so it doesn't appear to be a case of an unnoticed kaboom. > Will look tomorrow to see if we're not getting the expected stats on x64. It might not fail at all if there is no useful data stored 4 bytes prior to the variable passed in for the outparam. MSVC might just have dead data there. chrome_elf_unittests pass with Clang in release mode, despite this overwrite. https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:459: IN OUT PSIZE_T ProtectSize, On 2015/02/17 03:05:46, Will Harris wrote: > I think this should remain PSIZE_T Left as PSIZE_T
https://codereview.chromium.org/921353002/diff/1/chrome_elf/blacklist/blackli... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/921353002/diff/1/chrome_elf/blacklist/blackli... chrome_elf/blacklist/blacklist_interceptions.cc:98: SIZE_T bytes_returned; should this be PULONG now?
https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h File sandbox/win/src/nt_internals.h (right): https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); On 2015/02/19 01:57:06, Reid Kleckner wrote: > On 2015/02/17 23:54:43, robertshield wrote: > > On 2015/02/17 20:47:30, rvargas wrote: > > > Which means that if this is wrong, > > > GetBackingModuleFilePath should be failing pretty bad. > > > > Possibly. It could also accidentally work / silently fail most of the time > > depending on what got passed. We have a very small number of crashes around > this > > code, mostly on x86 so it doesn't appear to be a case of an unnoticed kaboom. > > Will look tomorrow to see if we're not getting the expected stats on x64. > > It might not fail at all if there is no useful data stored 4 bytes prior to the > variable passed in for the outparam. MSVC might just have dead data there. > chrome_elf_unittests pass with Clang in release mode, despite this overwrite. My point was that looking at the code of GetBackingModuleFilePath, I'd say that this should fail if the sizes are wrong. So to put it another way, I don't want to change any value from 64 to 32 bits or viceversa without someone actually making sure that the OS is really expecting what we pass. Given that we are using some of this code, I'm inclined to say that it works unless proven wrong. Note that for the first four arguments we have kind of a free pass as they go in registers (so the result depends on whatever was in the high part), but after that, sizes should match the stack use (I think).
On 2015/02/19 02:09:55, rvargas wrote: > https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h > File sandbox/win/src/nt_internals.h (right): > > https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... > sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); > On 2015/02/19 01:57:06, Reid Kleckner wrote: > > On 2015/02/17 23:54:43, robertshield wrote: > > > On 2015/02/17 20:47:30, rvargas wrote: > > > > Which means that if this is wrong, > > > > GetBackingModuleFilePath should be failing pretty bad. > > > > > > Possibly. It could also accidentally work / silently fail most of the time > > > depending on what got passed. We have a very small number of crashes around > > this > > > code, mostly on x86 so it doesn't appear to be a case of an unnoticed > kaboom. > > > Will look tomorrow to see if we're not getting the expected stats on x64. > > > > It might not fail at all if there is no useful data stored 4 bytes prior to > the > > variable passed in for the outparam. MSVC might just have dead data there. > > chrome_elf_unittests pass with Clang in release mode, despite this overwrite. > > My point was that looking at the code of GetBackingModuleFilePath, I'd say that > this should fail if the sizes are wrong. > > So to put it another way, I don't want to change any value from 64 to 32 bits or > viceversa without someone actually making sure that the OS is really expecting > what we pass. Given that we are using some of this code, I'm inclined to say > that it works unless proven wrong. Well, I have proof that VirtualQuery is wrong, given that the change fixes chrome_elf_unittests with x64 Clang in debug mode. I have no evidence for the others. Would you like me to revert the other changes, or investigate? I don't really want to block greening our bot on analyzing system call out param sizes. > Note that for the first four arguments we have kind of a free pass as they go in > registers (so the result depends on whatever was in the high part), but after > that, sizes should match the stack use (I think). For outparams, it's actually very important that the sizes match regardless of the position, or you have an overwrite or uninit read.
On 2015/02/19 19:26:59, Reid Kleckner wrote: > On 2015/02/19 02:09:55, rvargas wrote: > > > https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals.h > > File sandbox/win/src/nt_internals.h (right): > > > > > https://codereview.chromium.org/921353002/diff/1/sandbox/win/src/nt_internals... > > sandbox/win/src/nt_internals.h:454: OUT PSIZE_T ReturnLength OPTIONAL); > > On 2015/02/19 01:57:06, Reid Kleckner wrote: > > > On 2015/02/17 23:54:43, robertshield wrote: > > > > On 2015/02/17 20:47:30, rvargas wrote: > > > > > Which means that if this is wrong, > > > > > GetBackingModuleFilePath should be failing pretty bad. > > > > > > > > Possibly. It could also accidentally work / silently fail most of the time > > > > depending on what got passed. We have a very small number of crashes > around > > > this > > > > code, mostly on x86 so it doesn't appear to be a case of an unnoticed > > kaboom. > > > > Will look tomorrow to see if we're not getting the expected stats on x64. > > > > > > It might not fail at all if there is no useful data stored 4 bytes prior to > > the > > > variable passed in for the outparam. MSVC might just have dead data there. > > > chrome_elf_unittests pass with Clang in release mode, despite this > overwrite. > > > > My point was that looking at the code of GetBackingModuleFilePath, I'd say > that > > this should fail if the sizes are wrong. > > > > So to put it another way, I don't want to change any value from 64 to 32 bits > or > > viceversa without someone actually making sure that the OS is really expecting > > what we pass. Given that we are using some of this code, I'm inclined to say > > that it works unless proven wrong. > > Well, I have proof that VirtualQuery is wrong, given that the change fixes > chrome_elf_unittests with x64 Clang in debug mode. I have no evidence for the > others. Would you like me to revert the other changes, or investigate? I don't > really want to block greening our bot on analyzing system call out param sizes. I should have read the bug report!. There is also http://crbug.com/397137 referenced from blacklist_test.cc. So yes, that matches my expectation that if the size is wrong, using it gives an unexpected result. We can revert the other change for now while someone actually investigates... I'm a little surprised that both calls don't use the same size. If you have time to investigate that would be great! > > > Note that for the first four arguments we have kind of a free pass as they go > in > > registers (so the result depends on whatever was in the high part), but after > > that, sizes should match the stack use (I think). > > For outparams, it's actually very important that the sizes match regardless of > the position, or you have an overwrite or uninit read. I didn't mean to imply that it is not important, just that it may be harder to see it failing. And by fact of providing the function signature I don't think there would be an immediate overwrite on the caller side, right? We get a register from the kernel and our code will copy all or part of it as we instruct it to (yes, still wrong but no corruption). Thanks!
\> I should have read the bug report!. There is also http://crbug.com/397137 > referenced from blacklist_test.cc. So yes, that matches my expectation that if > the size is wrong, using it gives an unexpected result. > > We can revert the other change for now while someone actually investigates... Ricardo, did you see comment 10? (It makes more sense for that param to be a size_t, and dynamorio has one too. Since this is an outparam, an ULONG will work on little endian just fine as long as the number is less than 4 GB.) Reid, did you see comment 11?
On 2015/02/19 20:30:28, Nico wrote: > \> I should have read the bug report!. There is also http://crbug.com/397137 > > referenced from blacklist_test.cc. So yes, that matches my expectation that if > > the size is wrong, using it gives an unexpected result. > > > > We can revert the other change for now while someone actually investigates... > > Ricardo, did you see comment 10? (It makes more sense for that param to be a > size_t, and dynamorio has one too. Since this is an outparam, an ULONG will work > on little endian just fine as long as the number is less than 4 GB.) > > Reid, did you see comment 11? Yes, I did. NtQueryVirtualMemoryFunction is moving from ULONG to SIZE_T (for an info class structure so it's not really expected to be of any size). NtQuerySectionFunction is currently going from SIZE_T to ULONG for basically the same thing and that's what I'm saying is somewhat surprising (but I can accept anything from whoever wrote that code anyway)
> Yes, I did. NtQueryVirtualMemoryFunction is moving from ULONG to SIZE_T (for an > info class structure so it's not really expected to be of any size). > NtQuerySectionFunction is currently going from SIZE_T to ULONG for basically the > same thing and that's what I'm saying is somewhat surprising (but I can accept > anything from whoever wrote that code anyway) I think the argument was that one is the size of something on-disk from a PE file, while other is the size of something in memory. But I suppose I'd rather have our bot green than argue about this, so this cl lgtm either with the NtQueryVirtualMemoryFunction() change reverted or the thing in comment 11 addressed.
On 2015/02/19 20:45:27, Nico wrote: > > Yes, I did. NtQueryVirtualMemoryFunction is moving from ULONG to SIZE_T (for > an > > info class structure so it's not really expected to be of any size). > > NtQuerySectionFunction is currently going from SIZE_T to ULONG for basically > the > > same thing and that's what I'm saying is somewhat surprising (but I can accept > > anything from whoever wrote that code anyway) > > I think the argument was that one is the size of something on-disk from a PE > file, while other is the size of something in memory. But it's not. It's just an information class not tied to a PE file (with a LARGE_INTEGER to describe the size of the section itself, which would be pointless for a PE). > > But I suppose I'd rather have our bot green than argue about this, so this cl > lgtm either with the NtQueryVirtualMemoryFunction() change reverted or the thing > in comment 11 addressed.
From my perspective, this LGTM. The size change on NtQueryVirtualMemory is either correct, in which case crbug.com/397137 should be fixed, or it isn't in which case I don't believe this will make things worse and we can revert later. Will try and find some time tomorrow to investigate.
rvargas, does landing this as is (with comment 11 addressed) and reverting the QueryVirtualMem part if it doesn't help with crbug.com/397137 sound fine to you?
https://codereview.chromium.org/921353002/diff/1/chrome_elf/blacklist/blackli... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/921353002/diff/1/chrome_elf/blacklist/blackli... chrome_elf/blacklist/blacklist_interceptions.cc:98: SIZE_T bytes_returned; On 2015/02/19 02:07:09, Nico wrote: > should this be PULONG now? Huh, I did that locally to fix the build, but it didn't make the patch upload.
New patchsets have been uploaded after l-g-t-m from thakis@chromium.org,robertshield@chromium.org
upload again
On 2015/02/19 21:21:07, Reid Kleckner wrote: > upload again Err, looks like I commented on the wrong patch set. lgtm as-is, given Ricardo is cool with this.
Now I'm confused! I thought (by comment 13) that NtQueryVirtualMemoryFunction is the one that we have evidence is wrong (and that is the one implied in the previous bug report / disabled test). I'm fine changing that one. As for NtQuerySectionFunction, I don't believe we have evidence, do we? And if we make this change we are going from a safer version (longer args) to a less safe version (short arg that may be overwritten). Isn't it better to commit only the one we know is wrong? I'm fine as long as someone takes the time to see what the kernel does, really soon. I've seen related stuff languish for years. (BTW, the sandbox should be fine as this path is only exercised with certain policies that Chrome doesn't use ATM).
So I guess that means lgtm with caveats? (given that the direct user is elf)
OK, so I'm going to land as is, since both of the routines being changed (NtQuerySection and NtQueryVirtualMemory) are pretty core to DyanmoRIO, and it has run on Win64 and Win32 for a long time. To recap the effect of the change: NtQuerySection: SIZE_T -> ULONG NtQueryVirtualMemory: ULONG -> SIZE_T
The CQ bit was checked by rnk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/921353002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4ede4e174a51d08262edfb2e2c05845504bd9b1a Cr-Commit-Position: refs/heads/master@{#317177}
Message was sent while issue was closed.
Is it possible that this change caused the test failures on the Win7 bots in https://crbug.com/460563? What versions of Windows did you test this on?
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/943043002/ by jfb@chromium.org. The reason for reverting is: Speculative revert after discussion on IRC, this seems pretty unlikely but may have caused the failures at: http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
