Re: Memory corruption in out_batch parameter of batch lookup APIs

[Yonghong Song <yonghong.song@linux.dev>]

On 2/16/24 4:24 PM, Martin Kelly wrote:
> Hi, I noticed there's a subtlety to to the batch APIs 
> (bpf_map_lookup_batch and bpf_map_lookup_and_delete_batch) that can 
> lead to bugs if callers are not careful, and I'm wondering about the 
> best way to document or address it.
>
> Specifically, the size of the data pointed to by in_batch/out_batch is 
> not clear, and if it's too small, the caller can see memory/stack 
> corruption. The function documentation isn't super clear about this, 
> calling in_batch "address of the first element in batch to read", so a 
> caller might reasonably assume that a pointer size is fine. However, 
> the right size actually depends on the map type.
>
> For hash and array maps, out_batch will be u32 (as the parameter is 
> used as an index). But for LPM trie, it will be the size of the key 
> (in the case of LPM trie, I think that's 260 bytes). If a caller 
> passes a pointer to memory smaller the key size, the kernel will 
> overwrite past that memory and corrupt the stack (or wherever 
> out_batch points). This is because of the copy_to_user(uobatch, 
> prev_key, map->key_size) at the end of generic_map_lookup_batch.
>
> It seems to me that we could add documentation to these functions 
> indicating that out_batch should be able to hold at least one key to 
> be safe. This is simple but overly strict (at the moment) for all map 
> types other than LPM trie. However, if we specifically call out LPM 
> trie as needing key-sized width while other map types need 4 bytes, 
> then this documentation could easily become out-of-date as new map 
> types are added.
>
> We could alternatively add a statement like "out_batch should 
> generally point to memory large enough to hold a single key, but for 
> some map implementations a smaller type is possible". This gives more 
> information but might be too vague for many API users, and it means 
> future kernels could be tied to this implementation to avoid breaking 
> users.
>
> Any thoughts/preferences on how best to handle this? I'm happy to send 
> a patch clarifying the documentation, but I'd like to get a general 
> consensus on the best way to proceed first.

For in_batch/out_batch, the only exception is hashmap which has in_batch/out_batch memory size to be 32.
For all other supported maps, in_batch/out_batch memory size should be equal to their corresponding key size.
Maybe you can clarify this in include/uapi/linux/bpf.h?

We can update the documentation later if there are any exceptions to the above rule.

>
> Thanks,
>
> Martin
>