Re: [PATCH v1] pidfs: do not report fields that do not fit into the caller's struct

[Christian Brauner]

> The documentation of struct pidfd_info promises that "If the structure
> size is too small to contain a field (requested or not), to avoid
> confusion the mask will not contain a bit for that field", and asks
> userspace to trust the returned mask when deciding which fields hold
> valid data.
> 
> pidfd_info() does not keep that promise. It builds kinfo.mask without
> looking at the size encoded in the ioctl command, and
> copy_struct_to_user() then truncates the copy to that size. A caller
> that passes a 64-byte struct (PIDFD_INFO_SIZE_VER0) and requests
> PIDFD_INFO_COREDUMP or PIDFD_INFO_SUPPORTED_MASK gets a mask with
> PIDFD_INFO_COREDUMP, PIDFD_INFO_COREDUMP_SIGNAL, PIDFD_INFO_COREDUMP_CODE
> and PIDFD_INFO_SUPPORTED_MASK set, while the coredump_* and
> supported_mask fields behind the 64 bytes were never written. Trusting
> the mask as documented, userspace then reads whatever its own buffer
> held at those offsets. The same happens for a 72- or 80-byte struct
> and the fields added after it.
> 
> Clear the bits of the fields that lie beyond the caller's size before
> the copy, in the one place that knows both the final mask and the size.
> A request for a field that cannot be returned is thereby answered the
> way the header describes: the field is simply absent from the mask.
> 
> The added selftest issues PIDFD_GET_INFO with the first published size
> over a full-size, poisoned struct and checks that the mask names none
> of the fields beyond that size and that the poison is intact. It fails
> on an unpatched kernel and passes with this change.
> 
> Found by formal verification of fs/pidfs.c against the kernel binary.
> 
> Fixes: 1d8db6fd698d ("pidfs, coredump: add PIDFD_INFO_COREDUMP")
> Signed-off-by: Nickolai Zeldovich <nickolai@xxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner (Amutable) <brauner@xxxxxxxxxx>
> ---
> Notes:
>     The three size thresholds follow the field layout: coredump_mask and
>     coredump_signal end at PIDFD_INFO_SIZE_VER1 (72), coredump_code at
>     PIDFD_INFO_SIZE_VER2 (80), supported_mask at PIDFD_INFO_SIZE_VER3 (88).
>     Masking the request bits early instead (before the coredump and
>     supported_mask blocks run) would save a little work for such callers
>     but leaves the bits set in the task->mm fallback path; clearing the
>     result once before copy_struct_to_user() covers every way a bit can be
>     set. No stable tag: the only callers affected pass a struct size
>     smaller than the fields they request, and the consequence is a wrong
>     mask bit, not a crash or corruption.
> 
>  fs/pidfs.c                                    |  8 ++++
>  tools/testing/selftests/pidfd/pidfd.h         | 16 +++++++
>  .../testing/selftests/pidfd/pidfd_info_test.c | 48 +++++++++++++++++++

Semantic changes and selftest changes in separate patches, please.

--