advisory-db icon indicating copy to clipboard operation
advisory-db copied to clipboard

report unsound issue in bcc

Open llooFlashooll opened this issue 1 year ago • 3 comments

For more information, see: rust-bcc/issues/200 In the meantime, bcc will no longer be maintained. Users are encouraged to migrate to libbpf-rs.

llooFlashooll avatar Aug 21 '24 06:08 llooFlashooll

Hi, I think this safe function contains unsafe code which is the raw pointer. And conversion between raw pointer is unsound since it bypasses the Rust safety guarantee. It can create a misaligned pointer.

For example, by calling this function, we can construct this example. Besides, the generic T can be any other possible types. The u8 type can be cast into any other type, not just this case.

fn main() {
    let mut vec = vec![1u8];
    let ptr = null_or_mut_ptr::<u64>(&mut vec);
    unsafe {
        let value: u64 = *ptr;
    }
}

llooFlashooll avatar Aug 21 '24 10:08 llooFlashooll

Your example while indeed incorrect rust does not discredit null_or_mut_ptr. The source of the incorrectness is the unsafe block added in your example not null_or_mut_ptr. Pointers created in a safe manner aren't inherently unsafe, even if they might be nonsensical or easy to misuse in unsafe code. It's the responsibility of the unsafe code to not use the pointer incorrectly.

As fare as I can tell null_or_mut_ptr in rust-bcc is only used thrice. Twice as an argument to bpf_prog_load and once as an argument to bcc_prog_load. At a quick glance (didn't check all versions) the relevant argument is of type c_char in each case which is an alias for either u8 or i8 and as such layout compatible with u8.

Not involving a generic i.e. using c_char instead of a generic T would be better to not accidentally cause UB/unsoudness, but I don't see an immediate problem as is.

Skgland avatar Aug 21 '24 12:08 Skgland

Hi, thanks for your detailed and reasonable reply! I agree with your comment, and the unsafe consequence would depend on how the users use the code. Based on your description, can I switch to apply for an ID under the unmaintained category? Seems like this repo still needs further maintenance while doesn't.

llooFlashooll avatar Aug 22 '24 06:08 llooFlashooll

An unmaintained advisory has been published for bcc, so I think we can close this issue now, as the unsoundness is not clear.

amousset avatar Nov 10 '24 15:11 amousset