mm/madvise: reject invalid process_madvise() advice for zero-length vectors

process_madvise() used to validate the advice while walking each imported
iovec.  If the vector has zero total length, vector_madvise() does not
enter the loop and can return success without checking whether the advice
value is valid.

For a local mm, such as process_madvise(PIDFD_SELF, ...), the remote-only
process_madvise_remote_valid() check is skipped.  As a result, an invalid
advice can be reported as success when the vector has zero total length.
This differs from madvise(), which rejects an invalid advice before
returning success for a zero-length range.

Validate the generic madvise behavior at the syscall-facing entry points
before any vector walk.  In process_madvise(), do this before the
remote-only advice restriction so unsupported advice is rejected with the
same priority for local and remote mm.

Use an errno-returning helper for address/length validation, and handle
zero-length ranges explicitly at the call sites.  Requests with valid
advice and zero total length remain a noop and continue to return 0.  Add
a selftest that covers invalid advice with a zero-length iovec and an
empty vector, while also checking that a request with valid advice and
zero length still succeeds.

Link: https://lore.kernel.org/tencent_C3AEB0E769C5F4F9370F9411B69B7F8B2907@qq.com
Fixes: 021781b01275 ("mm/madvise: unrestrict process_madvise() for current process")
Signed-off-by: fujunjie <fujunjie1@qq.com>
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Reviewed-by: SeongJae Park <sj@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jann Horn <jannh@google.com>
Cc: Liam Howlett <liam@infradead.org>
Cc: Lorenzo Stoakes <ljs@kernel.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Vlastimil Babka <vbabka@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

diff --git a/mm/madvise.c b/mm/madvise.c
index 69708e953cf5603e58c26f192f674ecf17d6a34b..cd9bb077072ccb1b90242a3211a9c6ef48929f50 100644 (file)
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1834,50 +1834,29 @@ static void madvise_finish_tlb(struct madvise_behavior *madv_behavior)
                tlb_finish_mmu(madv_behavior->tlb);
 }
 
-static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
+/**
+ * check_input_range() - Check if the requested range is valid.
+ * @start:     Start address of madvise-requested address range.
+ * @len_in:    Length of madvise-requested address range.
+ *
+ * Returns: 0 if the input range is valid, otherwise an error code.
+ */
+static int check_input_range(unsigned long start, size_t len_in)
 {
        size_t len;
 
-       if (!madvise_behavior_valid(behavior))
-               return false;
-
        if (!PAGE_ALIGNED(start))
-               return false;
+               return -EINVAL;
        len = PAGE_ALIGN(len_in);
 
        /* Check to see whether len was rounded up from small -ve to zero */
        if (len_in && !len)
-               return false;
+               return -EINVAL;
 
        if (start + len < start)
-               return false;
-
-       return true;
-}
+               return -EINVAL;
 
-/*
- * madvise_should_skip() - Return if the request is invalid or nothing.
- * @start:     Start address of madvise-requested address range.
- * @len_in:    Length of madvise-requested address range.
- * @behavior:  Requested madvise behavior.
- * @err:       Pointer to store an error code from the check.
- *
- * If the specified behaviour is invalid or nothing would occur, we skip the
- * operation.  This function returns true in the cases, otherwise false.  In
- * the former case we store an error on @err.
- */
-static bool madvise_should_skip(unsigned long start, size_t len_in,
-               int behavior, int *err)
-{
-       if (!is_valid_madvise(start, len_in, behavior)) {
-               *err = -EINVAL;
-               return true;
-       }
-       if (start + PAGE_ALIGN(len_in) == start) {
-               *err = 0;
-               return true;
-       }
-       return false;
+       return 0;
 }
 
 static bool is_madvise_populate(struct madvise_behavior *madv_behavior)
@@ -2013,8 +1992,13 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
                .tlb = &tlb,
        };
 
-       if (madvise_should_skip(start, len_in, behavior, &error))
+       if (!madvise_behavior_valid(behavior))
+               return -EINVAL;
+
+       error = check_input_range(start, len_in);
+       if (error || !len_in)
                return error;
+
        error = madvise_lock(&madv_behavior);
        if (error)
                return error;
@@ -2056,7 +2040,8 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter,
                size_t len_in = iter_iov_len(iter);
                int error;
 
-               if (madvise_should_skip(start, len_in, behavior, &error))
+               error = check_input_range(start, len_in);
+               if (error || !len_in)
                        ret = error;
                else
                        ret = madvise_do_behavior(start, len_in, &madv_behavior);
@@ -2131,6 +2116,11 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
                goto release_task;
        }
 
+       if (!madvise_behavior_valid(behavior)) {
+               ret = -EINVAL;
+               goto release_mm;
+       }
+
        /*
         * We need only perform this check if we are attempting to manipulate a
         * remote process's address space.

diff --git a/tools/testing/selftests/mm/process_madv.c b/tools/testing/selftests/mm/process_madv.c
index cd4610baf5d7d2e962fda69cb376a4fdda16fbcd..3fffd5f7e6fb4ae0bb59c127010deddee6975436 100644 (file)
--- a/tools/testing/selftests/mm/process_madv.c
+++ b/tools/testing/selftests/mm/process_madv.c
@@ -309,6 +309,34 @@ TEST_F(process_madvise, invalid_vlen)
        ASSERT_EQ(munmap(map, pagesize), 0);
 }
 
+/*
+ * Test that invalid advice is rejected even when the iovec has zero total
+ * length. A request with valid advice and zero length is a noop, but
+ * invalid advice should still fail with EINVAL.
+ */
+TEST_F(process_madvise, invalid_advice_zero_length)
+{
+       struct iovec vec = {
+               .iov_base = NULL,
+               .iov_len = 0,
+       };
+       int pidfd = self->pidfd;
+       ssize_t ret;
+
+       errno = 0;
+       ret = sys_process_madvise(pidfd, &vec, 1, -1, 0);
+       ASSERT_EQ(ret, -1);
+       ASSERT_EQ(errno, EINVAL);
+
+       errno = 0;
+       ret = sys_process_madvise(pidfd, &vec, 1, MADV_DONTNEED, 0);
+       ASSERT_EQ(ret, 0);
+
+       ret = sys_process_madvise(pidfd, NULL, 0, -1, 0);
+       ASSERT_EQ(ret, -1);
+       ASSERT_EQ(errno, EINVAL);
+}
+
 /*
  * Test process_madvise() with an invalid flag value. Currently, only a flag
  * value of 0 is supported. This test is reserved for the future, e.g., if