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

[saw-remote-api] [test] tests/saw/test_salsa20.py: ExpandContract should allocate pointer arguments as read-only #1280

Closed
WeeknightMVP opened this issue Apr 29, 2021 · 1 comment · Fixed by #1282

Comments

@WeeknightMVP
Copy link

WeeknightMVP commented Apr 29, 2021

If we change the size to 65 (or more) when verifying that s20_crypt32 meets Salsa20CryptContract...

class Salsa20EasyTest(unittest.TestCase):
    def test_salsa20(self):
        # ...
        crypt_result = llvm_verify(mod, 's20_crypt32', Salsa20CryptContract(65), lemmas=[expand_result])
        # ...

..., verification fails due to a failed override assumption:

saw-script/saw-remote-api/python$ poetry run python tests/saw/test_salsa20.py
...

⚠️  Failed to verify: lemma_Salsa20CryptContract (defined at .../saw-script/saw-remote-api/python/tests/saw/test_salsa20.py:110):
error: user error (Symbolic execution failed.
Abort due to false assumption:
  salsa20.c:182:7: error: in s20_crypt32
  All overrides failed during structural matching:
  *  Name: s20_expand32
     Location: internal
     Argument types:
     - i8*
     - i8*
     - i8*
     Return type: <void>
     Arguments:
     - concrete pointer: allocation = 15, offset = 0
     - concrete pointer: allocation = 25, offset = 0
     - concrete pointer: allocation = 24, offset = 0
     at internal
     error when loading through pointer that appeared in the override's points-to precondition(s):
     Precondition:
       Pointer: concrete pointer: allocation = 25, offset = 0
       Pointee: fresh:x0#670 : [16][8]

       Assertion made at: internal
     Failure reason:
       When reading through pointer: (25, 0x0:[64])
       in the  precondition of an override
       Tried to read something of size: 16
       And type: [16 x i8]
       Found 1 possibly matching allocation(s):
       - StackAlloc 25 0x10:[64] Mutable 16-byte-aligned salsa20.c:155:11
Stack frame s20_crypt32

Amending ExpandContract to allocate memory for pointer arguments as read-only...

class ExpandContract(Contract):
    def specification(self):
        # ...
        k_p = self.alloc(LLVMArrayType(i8, 32), read_only = True)  # <--
        n_p = self.alloc(LLVMArrayType(i8, 16), read_only = True)  # <--
        # ...

...corrects this:

...

✅  Verified: lemma_Salsa20CryptContract (defined at .../saw-script/saw-remote-api/python/tests/saw/test_salsa20.py:110)

...
@RyanGlScott
Copy link
Contributor

Good catch. In fact, we already use llvm_alloc_readonly in the SAWScript proofs for salsa20, but for whatever reason, the Python version didn't take after this. This should be straightforward to fix.

RyanGlScott added a commit that referenced this issue Apr 30, 2021
In addition, test `Salsa20CryptContract` on more inputs, just like the
SAWScript version does, to make sure that it is working robustly.

Fixes #1280.
@mergify mergify bot closed this as completed in #1282 Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants