Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix out of bounds accesses for invalid vertex component formats #12684

Merged

Conversation

Pokechu22
Copy link
Contributor

On all platforms, this would result in out of bounds accesses when getting the component sizes (which uses stuff from VertexLoader_Position.h/VertexLoader_TextCoord.h/VertexLoader_Normal.h). On platforms other than x64 and ARM64, this would also be out of bounds accesses when getting function pointers for the non-JIT vertex loader (in VertexLoader_Position.cpp etc.). Usually both of these would get data from other entries in the same multi-dimensional array, but the last few entries would be truly out of bounds. This does mean that an out of bounds function pointer can be called on platforms that don't have a JIT vertex loader, but it is limited to invalid component formats with values 5/6/7 due to the size of the bitfield the formats come from, so it seems unlikely that this could be exploited in practice.

This issue affects a few games; Def Jam: Fight for New York (https://bugs.dolphin-emu.org/issues/12719) and Fifa Street are known to be affected.

I have not done any hardware testing for this PR specifically, though I think I previously determined that at least a value of 5 behaves the same as float (4). That's what I implemented in any case. I did previously determine that both Def Jam: Fight for New York and Fifa Street use an invalid normal format, but don't actually have lighting enabled when that normal vector is used, so it doesn't change rendering in practice.

The color component format also has two invalid values, but VertexLoader_Color.h/.cpp do check for those invalid ones and return a default value instead of doing an out of bounds access.

static DOLPHIN_FORCE_INLINE u32 GetSize(VertexComponentFormat type, ColorFormat format)
{
if (format > ColorFormat::RGBA8888)
{
PanicAlertFmt("Invalid color format {}", format);
return 0;
}
return s_table_size[type][format];
}

TPipelineFunction VertexLoader_Color::GetFunction(VertexComponentFormat type, ColorFormat format)
{
if (format > ColorFormat::RGBA8888)
{
PanicAlertFmt("Invalid color format {}", format);
return nullptr;
}
return s_table_read_color[type][format];
}

@iwubcode
Copy link
Contributor

iwubcode commented Apr 2, 2024

I know Def Jam is known for its brokenness, I don't know about the other game, but this change is surprising to me (then again Red Steel audio issue was a huge surprise too). Should we add a game quirk to see if any other games exhibit this issue?

@AdmiralCurtiss
Copy link
Contributor

Sounds like a reasonable approach, the native format uses 3 bits for reference. (ComponentFormat in CPMemory.h)

@Pokechu22
Copy link
Contributor Author

I added a game quirk.

@AdmiralCurtiss
Copy link
Contributor

Wait, you did something wrong with the rcheevos external in the rebase.

@AdmiralCurtiss AdmiralCurtiss self-requested a review April 4, 2024 19:42
On all platforms, this would result in out of bounds accesses when getting the component sizes (which uses stuff from VertexLoader_Position.h/VertexLoader_TextCoord.h/VertexLoader_Normal.h). On platforms other than x64 and ARM64, this would also be out of bounds accesses when getting function pointers for the non-JIT vertex loader (in VertexLoader_Position.cpp etc.). Usually both of these would get data from other entries in the same multi-dimensional array, but the last few entries would be truly out of bounds. This does mean that an out of bounds function pointer can be called on platforms that don't have a JIT vertex loader, but it is limited to invalid component formats with values 5/6/7 due to the size of the bitfield the formats come from, so it seems unlikely that this could be exploited in practice.

This issue affects a few games; Def Jam: Fight for New York (https://bugs.dolphin-emu.org/issues/12719) and Fifa Street are known to be affected.

I have not done any hardware testing for this PR specifically, though I *think* I previously determined that at least a value of 5 behaves the same as float (4). That's what I implemented in any case. I did previously determine that both Def Jam: Fight for New York and Fifa Street use an invalid normal format, but don't actually have lighting enabled when that normal vector is used, so it doesn't change rendering in practice.

The color component format also has two invalid values, but VertexLoader_Color.h/.cpp do check for those invalid ones and return a default value instead of doing an out of bounds access.
@Pokechu22
Copy link
Contributor Author

I forgot to update the submodule when I pulled on master, and then when I amended my previous commit the old submodule version was still in use, oops.

@AdmiralCurtiss AdmiralCurtiss merged commit ad33120 into dolphin-emu:master Apr 4, 2024
11 checks passed
@dolphin-ci
Copy link

dolphin-ci bot commented Apr 4, 2024

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • fifa-street on ogl-lin-mesa: diff
  • fifa-street on sw-lin-mesa: diff
  • fifa-street on mvk-osx-m1: diff
  • fifa-street on mtl-osx-m1: diff

automated-fifoci-reporter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants