Jesper Dangaard Brouer <hawk@xxxxxxxxxx> writes: > On 21/02/2024 15.48, Toke Høiland-Jørgensen wrote: >> Toke Høiland-Jørgensen <toke@xxxxxxxxxx> writes: >> >>> The cookie is a random 128-bit value, which means the probability that >>> we will get accidental collisions (which would lead to recycling the >>> wrong page values and reading garbage) is on the order of 2^-128. This >>> is in the "won't happen before the heat death of the universe" range, so >>> this marking is safe for the intended usage. >> >> Alright, got a second opinion on this from someone better at security >> than me; I'll go try out some different ideas :) > > It is a general security concern for me that BPF test_run gets access to > memory used by 'system page pool', with the concern of leaking data > (from real traffic) to an attacker than can inject a BPF test_run > program via e.g. a CI pipeline. > > I'm not saying we leaking data today in BPF/XDP progs, but there is a > potential, because to gain performance in XDP and page_pool we don't > clear memory to avoid cache line performance issues. > I guess today, I could BPF tail extend and read packet data from older > frames, in this way, if I get access to 'system page pool'. I agree that the leak concern is non-trivial (also of the secret cookie value), so I am not planning to re-submit with that approach. I got half-way revising the patches to use the system PP but always re-initialise the pages before the merge window. This comes with a ~7% overhead on small packets and probably more with big frames (due to the memcpy() of the payload data). Due to this, my current plan is to take a hybrid approach, depending on the 'repetitions' parameter: for low repetition counts, just pre-allocate a bunch of pages from the system PP at setup time, initialise them, and don't bother with recycling. And for large repetition counts, keep the current approach of allocating a separate PP for each syscall invocation. The threshold for when something is a "low" number is the kicker here, of course, but probably some static value can be set as a threshold; I'll play around with this and see what makes sense. WDYT about this? -Toke
