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

[Bug] Out-of-bounds writes in BitView::_Read64<true> potentially causing crashes or infinite loops #354

Open
chiayy-com opened this issue Jul 23, 2023 · 6 comments · May be fixed by #355
Open

Comments

@chiayy-com
Copy link

In the code (In the BitView::_Read64 method withCheckAlignment==true)

bladebit/src/util/BitView.h

Lines 156 to 163 in c669876

// No guarantee that the last field is complete, so copy only the bytes we know we have
const size_t totalBytes = CDiv( sizeBits, 8 );
const int32 remainderBytes = (int32)( totalBytes - fieldIndex * 8 );
field = 0;
byte* fieldBytes = (byte*)&field;
for( int32 i = 0; i < remainderBytes; i++ )
fieldBytes[i] = pField[i];

bladebit/src/util/BitView.h

Lines 186 to 192 in c669876

const size_t totalBytes = CDiv( sizeBits, 8 );
const int32 remainderBytes = (int32)( totalBytes - fieldIndex * 8 );
field = 0;
byte* fieldBytes = (byte*)&field;
for( int32 i = 0; i < remainderBytes; i++ )
fieldBytes[i] = pField[i];

const size_t totalBytes     = CDiv( sizeBits, 8 );
const int32  remainderBytes = (int32)( totalBytes - fieldIndex * 8 );

field = 0;
byte* fieldBytes = (byte*)&field;
for( int32 i = 0; i < remainderBytes; i++ )
    fieldBytes[i] = pField[i];

The calculation of remainderBytes is wrong. Which causes the value of remainderBytes to exceeds 8 potentially.
So fieldBytes[i] may reference to some dangers zone, resulting in undefined behavior.

FYI:
An example I encountered is that remainderBytes==12, and fieldBytes[8] happens to references to i, causing infinite loop. (built on Windows, debug mode)

@chiayy-com chiayy-com linked a pull request Jul 23, 2023 that will close this issue
@harold-b
Copy link
Contributor

const size_t totalBytes     = CDiv( sizeBits, 8 );
const int32  remainderBytes = (int32)( totalBytes - fieldIndex * 8 );

I have not gone through this code in a while, but this seems to be correct. It would never exceed 8 bytes if the correct sizeBits and position was passed. Perhaps add an ASSERT( position < sizeBits ); wherever you're using this.
Or if there is a bug, it could lie elsewhere, unless I'm missing something here.

@chiayy-com
Copy link
Author

chiayy-com commented Jul 25, 2023

https://github.com/Chia-Network/bladebit/blob/c669876eff68a29c679e3ff501a3ab656adf30b9/src/util/BitView.h#L127C1-L169

Consider evaluating _Read64<true>(bitCount, someAddressNonAligned, 0, 96);

You start with

sizeBits = 96;
position = 0;

so you get

totalBytes = ceil(96 / 8) = 12;
fieldIndex = floor(0 / 64) = 0;
isLastField = (0 == floor(96 / 64) - 1) = true;

remainderBytes = 12 - 0 * 8 = 12

I encountered this bug when I tried to fetch proof from a plot created with some old plotter, which is valid used with chiapos, but causes bladebit to crash.

@harold-b
Copy link
Contributor

OK, so it seems the bug would be in isLastField (no - 1 necessary), not in the remainderBytes calculation. If you can modify the PR to reflect that then perhaps we can review to ensure everything else is also correct.

@chiayy-com
Copy link
Author

chiayy-com commented Jul 26, 2023

OK. I'd like to do the modification.

BTW, would you like it if I also simplify these lines this time?

bladebit/src/util/BitView.h

Lines 150 to 153 in c669876

if( isPtrAligned && !isLastField )
field = *((uint64*)pField);
else if( !isLastField )
memcpy( &field, pField, sizeof( uint64 ) );

from

if( isPtrAligned && !isLastField )
    field = *((uint64*)pField);
else if( !isLastField )
    memcpy( &field, pField, sizeof( uint64 ) );

to

if( !isLastField )
    field = *((uint64*)pField);

This original optimization seems unnecessary. A 64-bit dereference is a single instruction under most architectures (unless not 64-bit). Though it'll be a slower instruction if the address is unaligned, there's no reason a memset could do it faster.

AFAIK, a memset is typically a byte-by-byte copy loop for <=8 data, possibly optimized using some specialized instruction set, but still won't over-perform a simple read. Feel free to correct me if I missed something.

@harold-b
Copy link
Contributor

Yes, please go ahead. At the time that was written I had incorrectly assumed it was invalid to read/write unaligned in arm64 as it was in the 32bit instruction set.

@jmhands
Copy link

jmhands commented Aug 2, 2023

@harold-b if this PR happened we can close this issue

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

Successfully merging a pull request may close this issue.

4 participants