[Validator] Check size of PSV. #6924

python3kgae · 2024-09-19T14:33:52Z

Check size of PSV part matches the PSVVersion.
Updated DxilPipelineStateValidation::ReadOrWrite to read based on initInfo.PSVVersion.
And return fail when size mismatch in RWMode::Read.

Fixes #6817

Check size of PSV part matches the PSVVersion. Updated DxilPipelineStateValidation::ReadOrWrite to read based on initInfo.PSVVersion. And return fail when size mismatch in RWMode::Read. Fixes microsoft#6817

tex3d

Should not block loading of newer PSV versions. Need to do this only in the validator code.

tex3d · 2024-09-19T17:09:11Z

include/dxc/DxilContainer/DxilPipelineStateValidation.h

@@ -1078,6 +1083,9 @@ DxilPipelineStateValidation::ReadOrWrite(const void *pBits, uint32_t *pSize,
  if (mode == RWMode::CalcSize) {
    *pSize = rw.GetSize();
    m_pPSVRuntimeInfo1 = nullptr; // clear ptr to tempRuntimeInfo
+  } else if (mode == RWMode::Read) {
+    if (rw.GetSize() != rw.GetOffset())
+      return false;


We don't want to fail the load when the size isn't equal to the version, otherwise, the forward compatibility will be broken. We only want to verify this in the validator, to ensure that we only validate known versions of PSV.

Changed to return rw.GetOffset() and do the compare in validator.

We'll fail the load when PSVVersion > MAX_PSV_VERSION anyway.
So, fail the load when size not match should be fine as well.

PSVVersion could send as default. So fail the load will not work :(

tex3d · 2024-09-19T19:40:12Z

include/dxc/DxilContainer/DxilPipelineStateValidation.h

+  if (initInfo.PSVVersion > 1)
+    AssignDerived(&m_pPSVRuntimeInfo2, m_pPSVRuntimeInfo0,
+                  m_uPSVRuntimeInfoSize); // failure ok
+  if (initInfo.PSVVersion > 2)


These should be unnecessary. The scenarios are:

CalcSize or Write mode: m_uPSVRuntimeInfoSize based on PSVVersion in initInfo, AssignDerived will only assign when struct fits in size.

Read mode: m_uPSVRuntimeInfoSize determines the struct version available, PSVVersion in initInfo is not used for this, and there is no independent source for PSVVersion for PSV readers.

Validator will want to validate that m_uPSVRuntimeInfoSize matches the one computed by initInfo.RuntimeInfoSize() where initInfo.PSVVersion has been set according to validator version. It's probably best if this is done manually by the validator before invoking InitFromPSV0, since an incorrect size will run that off the rails in a way that might just result in failure to load (malformed), or maybe not any error depending on the data and sizes, rather than a more specific error that we could emit in the validator. If this size is wrong, there's no point in trying to read anything else.

tex3d · 2024-09-19T19:45:27Z

include/dxc/DxilContainer/DxilPipelineStateValidation.h

@@ -1078,6 +1086,8 @@ DxilPipelineStateValidation::ReadOrWrite(const void *pBits, uint32_t *pSize,
  if (mode == RWMode::CalcSize) {
    *pSize = rw.GetSize();
    m_pPSVRuntimeInfo1 = nullptr; // clear ptr to tempRuntimeInfo
+  } else if (mode == RWMode::Read) {
+    *pSize = rw.GetOffset();


We should not need this, as the passed in size when reading is the size of the container part.
CalcSize mode should be sufficient for validating the container part size is that calculated size, aligned, and any padding for alignment is filled with zero.

Reverting this eliminates the need to add GetOffset() as well.

tex3d · 2024-09-19T19:56:30Z

lib/DxilValidation/DxilContainerValidation.cpp

  DxilPipelineStateValidation PSV;
-  if (!PSV.InitFromPSV0(pPSVData, PSVSize)) {
+  uint32_t PSVSizeRead = PSVSize;
+  if (!PSV.InitFromPSV0(pPSVData, &PSVSizeRead, PSVVersion)) {


Before we do this, we should check that m_uPSVRuntimeInfoSize (first 4 bytes of data) matches expected size for PSVVersion (PSVInitInfo(PSVVersion).RuntimeInfoSize()), reporting error and aborting if not.

Then, there's no need to pass PSVVersion to InitFromPSV0, and that overload can be removed/reverted.

We should also probably do some additional manual validation of sizes/strides based on the format before diving into the full InitFromPSV0:

size_t offset = 4

start by checking that the offset + PSVRuntimeInfo_size <= PSVSizeRead

map the appropriate PSV structures fitting in size manually to pPSVData + offset. offset += PSVRuntimeInfo_size

if ResourceCount > 0:

next 4 bytes is PSVResourceBindInfo_size, which must be == PSVInitInfo(PSVVersion).ResourceBindInfoSize(). offset += 4

check that offset + PSVResourceBindInfo_size * ResourceCount <= PSVSizeRead. offset += PSVResourceBindInfo_size * ResourceCount

if pPSVRuntimeInfo1:

offset + 4 <= PSVSizeRead. Read uint32_t StringTableSize. offset += 4

if StringTableSize non-zero:

StringTableSize must be 4-aligned, offset + StringTableSize <= PSVSizeRead, pPSVData + offset + StringTableSize - 1 must be null char \0. offset += StringTableSize

offset + 4 <= PSVSizeRead. Read uint32_t SemanticIndexTableEntries. offset += 4

offset + (SemanticIndexTableEntries * 4) <= PSVSizeRead. offset += SemanticIndexTableEntries * 4

If SigInputElements || SigOutputElements || SigPatchConstOrPrimElements:

offset + 4 <= PSVSizeRead. Read uint32_t PSVSignatureElement_size. offset += 4

PSVSignatureElement_size == PSVInitInfo(PSVVersion).SignatureElementSize()

offset + PSVSignatureElement_size * (SigInputElements + SigOutputElements + SigPatchConstOrPrimElements) <= PSVSizeRead

After this, it's possible to manually go through the ViewID data calculation, but not much you can report other than it being malformed if the overall size isn't correct, since there are no more specific size values in this part of the data that can be independently verified. Since InitFromPSV0 will already do that, you can just move on I think.

If anything fails in here, we should not attempt to continue with more checks or load the PSV0 data.

It's a lot, I know, but doing this is necessary to prevent incorrect sizes for records, which can allow either extra unvalidated embedded data, or result in general malformed error when we could be more helpful.

Note how the ReadOrWrite method uses the CheckedReaderWriter to ease all these calculations and checks. It would probably be best to write some helper methods similar to that, but with proper error reporting for the validator. For instance, it's useful to have a method that checks the remaining size before loading some value from the current offset, then increments the offset by the size of that value. You could simply use the CheckedReaderWriter, but there will be issues with the way errors are handled that might make it hard to report the appropriate error.

Working on this.

tex3d

In addition to the comments, we should be checking the string and index tables more thoroughly:

check that string table is well formed - size is aligned, buffer ends with \0
check that accesses to string table entries are in-bounds, before using the value.
check that accesses to index table entries are in-bounds, and that the (index+1)+size is within dword size of index table, before using the index array.

Then use PSVInitInfo to calculate correct size of PSV.

github-actions · 2024-09-19T22:28:54Z

✅ With the latest revision this PR passed the C/C++ code formatter.

[Validator] Check size of PSV.

42c73c8

Check size of PSV part matches the PSVVersion. Updated DxilPipelineStateValidation::ReadOrWrite to read based on initInfo.PSVVersion. And return fail when size mismatch in RWMode::Read. Fixes microsoft#6817

python3kgae requested a review from a team as a code owner September 19, 2024 14:33

python3kgae mentioned this pull request Sep 19, 2024

[Validator] Check content of PSV0 part in validation #6859

Merged

tex3d requested changes Sep 19, 2024

View reviewed changes

Return the size read for PSV for validation.

6ec774c

tex3d reviewed Sep 19, 2024

View reviewed changes

tex3d requested changes Sep 19, 2024

View reviewed changes

Check PSVRuntimeInfoSize early.

b7c81b4

Then use PSVInitInfo to calculate correct size of PSV.

python3kgae added 5 commits September 19, 2024 21:13

Make sure data in PSV part follows format before load.

69a78ef

Load PSV after check size.

fe34412

Add check for StringTable and SemanticIndexTable.

cafe1d9

Test out of bound access for StringTable.

9c4b132

Fix clang build issues.

69d12d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Check size of PSV. #6924

[Validator] Check size of PSV. #6924

python3kgae commented Sep 19, 2024

tex3d left a comment

tex3d Sep 19, 2024

python3kgae Sep 19, 2024

python3kgae Sep 19, 2024

python3kgae Sep 19, 2024

tex3d Sep 19, 2024

tex3d Sep 19, 2024

tex3d Sep 19, 2024

tex3d Sep 19, 2024

tex3d Sep 19, 2024

python3kgae Sep 19, 2024

python3kgae Sep 20, 2024

tex3d left a comment

github-actions bot commented Sep 19, 2024 •

edited

Loading

[Validator] Check size of PSV. #6924

Are you sure you want to change the base?

[Validator] Check size of PSV. #6924

Conversation

python3kgae commented Sep 19, 2024

tex3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tex3d left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024 • edited Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading