On 2025/3/19 4:55, Toke Høiland-Jørgensen wrote:
> Yunsheng Lin <linyunsheng@xxxxxxxxxx> writes:
>
>> On 2025/3/17 23:16, Toke Høiland-Jørgensen wrote:
>>> Yunsheng Lin <yunshenglin0825@xxxxxxxxx> writes:
>>>
>>>> On 3/14/2025 6:10 PM, Toke Høiland-Jørgensen wrote:
>>>>
>>>> ...
>>>>
>>>>>
>>>>> To avoid having to walk the entire xarray on unmap to find the page
>>>>> reference, we stash the ID assigned by xa_alloc() into the page
>>>>> structure itself, using the upper bits of the pp_magic field. This
>>>>> requires a couple of defines to avoid conflicting with the
>>>>> POINTER_POISON_DELTA define, but this is all evaluated at compile-time,
>>>>> so does not affect run-time performance. The bitmap calculations in this
>>>>> patch gives the following number of bits for different architectures:
>>>>>
>>>>> - 24 bits on 32-bit architectures
>>>>> - 21 bits on PPC64 (because of the definition of ILLEGAL_POINTER_VALUE)
>>>>> - 32 bits on other 64-bit architectures
>>>>
>>>> From commit c07aea3ef4d4 ("mm: add a signature in struct page"):
>>>> "The page->signature field is aliased to page->lru.next and
>>>> page->compound_head, but it can't be set by mistake because the
>>>> signature value is a bad pointer, and can't trigger a false positive
>>>> in PageTail() because the last bit is 0."
>>>>
>>>> And commit 8a5e5e02fc83 ("include/linux/poison.h: fix LIST_POISON{1,2}
>>>> offset"):
>>>> "Poison pointer values should be small enough to find a room in
>>>> non-mmap'able/hardly-mmap'able space."
>>>>
>>>> So the question seems to be:
>>>> 1. Is stashing the ID causing page->pp_magic to be in the mmap'able/
>>>> easier-mmap'able space? If yes, how can we make sure this will not
>>>> cause any security problem?
>>>> 2. Is the masking the page->pp_magic causing a valid pionter for
>>>> page->lru.next or page->compound_head to be treated as a vaild
>>>> PP_SIGNATURE? which might cause page_pool to recycle a page not
>>>> allocated via page_pool.
>>>
>>> Right, so my reasoning for why the defines in this patch works for this
>>> is as follows: in both cases we need to make sure that the ID stashed in
>>> that field never looks like a valid kernel pointer. For 64-bit arches
>>> (where CONFIG_ILLEGAL_POINTER_VALUE), we make sure of this by never
>>> writing to any bits that overlap with the illegal value (so that the
>>> PP_SIGNATURE written to the field keeps it as an illegal pointer value).
>>> For 32-bit arches, we make sure of this by making sure the top-most bit
>>> is always 0 (the -1 in the define for _PP_DMA_INDEX_BITS) in the patch,
>>> which puts it outside the range used for kernel pointers (AFAICT).
>>
>> Is there any season you think only kernel pointer is relevant here?
>
> Yes. Any pointer stored in the same space as pp_magic by other users of
> the page will be kernel pointers (as they come from page->lru.next). The
> goal of PP_SIGNATURE is to be able to distinguish pages allocated by
> page_pool, so we don't accidentally recycle a page from somewhere else.
> That's the goal of the check in page_pool_page_is_pp():
>
> (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE
>
> To achieve this, we must ensure that the check above never returns true
> for any value another page user could have written into the same field
> (i.e., into page->lru.next). For 64-bit arches, POISON_POINTER_DELTA
POISON_POINTER_DELTA is defined according to CONFIG_ILLEGAL_POINTER_VALUE,
if CONFIG_ILLEGAL_POINTER_VALUE is not defined yet, POISON_POINTER_DELTA
is defined to zero.
It seems only the below 64-bit arches define CONFIG_ILLEGAL_POINTER_VALUE
through grepping:
a29815a333c6 core, x86: make LIST_POISON less deadly
5c178472af24 riscv: define ILLEGAL_POINTER_VALUE for 64bit
f6853eb561fb powerpc/64: Define ILLEGAL_POINTER_VALUE for 64-bit
bf0c4e047324 arm64: kconfig: Move LIST_POISON to a safe value
The below 64-bit arches don't seems to define the above config yet:
MIPS64, SPARC64, System z(S390X),loongarch
Does ID stashing cause problem for the above arches?
> serves this purpose. For 32-bit arches, we can leave the top-most bits
> out of PP_MAGIC_MASK, to make sure that any valid pointer value will
> fail the check above.
The above mainly explained how to ensure page_pool_page_is_pp() will
not return false positive result from the page_pool perspective.
