From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CF79FC433FE for ; Fri, 30 Sep 2022 22:34:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 444528D0002; Fri, 30 Sep 2022 18:34:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 3F3C58D0001; Fri, 30 Sep 2022 18:34:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2BBA98D0002; Fri, 30 Sep 2022 18:34:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 1DCEA8D0001 for ; Fri, 30 Sep 2022 18:34:25 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id DC0AEC0252 for ; Fri, 30 Sep 2022 22:34:24 +0000 (UTC) X-FDA: 79970206848.22.1D6A98E Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by imf24.hostedemail.com (Postfix) with ESMTP id 737B9180012 for ; Fri, 30 Sep 2022 22:34:24 +0000 (UTC) Received: by mail-ej1-f41.google.com with SMTP id a26so11815895ejc.4 for ; Fri, 30 Sep 2022 15:34:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=7eVHcRj7bOTZCSoAUca2KGtBBs0g8lofOLyuo3DNi08=; b=OWwpfDo6nJkLj4c8oYWeKltUAPO4XgrAWv5U21zYbvxXgNBnsly/xqgtAcH6HhHyJN vqBPJy/094FFPg7DCLf+lmx2L3O8IHvFpy7X4hKfGJlgCFTdhSOwXv5v/1A+a2GxqbNd qdeLPj9oLCE2DHblUdargmzdWwfH+FyQN8U3ncL+KvcyWP9sp2DT4i9gfNCh0Gb1pzdg STOT/gBd3INNdDH6ml/3n0OpMEK4yClvJzTrft4YqX/Mmux+lH7AII5rAP8hK8S4hosM bH5DC06p9s4vNSKvmp+mvtQ+FSb96Pakn21GBFqGpneG+BpIHRqVOy0rxLl02wVB+bWE n9fw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7eVHcRj7bOTZCSoAUca2KGtBBs0g8lofOLyuo3DNi08=; b=ZELhN1AXt5cUK3z+7rsNOup8WZvpBkpF4H4xo1JqLqml/p1IpYAXbfAxERQk4ptZMn VkCpxej/Lldmo+oVobVBWt5w8gC6U0yCqUI7wX0Pcci4DXlZcNh0Y67qax2w6mA1tTu+ tYHNMwBTjEVQHz01RfZDP+4l03ciXW+VDXm7aT7O4SJYEQGy/fHFEEG3lDZ6tOTl4vhu wBJAFngMxVkMSgkTKSUH3mhvue2md+do1J4bgv0scm/i3BvUb0/oEwsexE6cBqPObWHo Y4K+pGHM2oMx6dGTSCs/QyT0mUZSYVxFp4zfWVRSf/fb3K0U7Z2FLh4UipYnExaccXtd 5wvg== X-Gm-Message-State: ACrzQf3tIFJyA1t+q/T3bb+FqmjXPVCT+lbwbo7raW3BHkWVAcr7zuUP XyVUetvwnSyAm6hRZAji3GWL2MQiUOXhmdESvfI= X-Google-Smtp-Source: AMsMyM5oLkCTD4Mgre7YQ5XEM/Vy8tcOUer5mjeC7MxutJyVNVlaxAGdlZ1ZTKYuy0vKQfNQitRS91hfcc157zmBbOg= X-Received: by 2002:a17:907:984:b0:77f:4d95:9e2f with SMTP id bf4-20020a170907098400b0077f4d959e2fmr8032846ejc.176.1664577262903; Fri, 30 Sep 2022 15:34:22 -0700 (PDT) MIME-Version: 1.0 References: <20220930003844.1210987-1-cmllamas@google.com> In-Reply-To: <20220930003844.1210987-1-cmllamas@google.com> From: Andrii Nakryiko Date: Fri, 30 Sep 2022 15:34:10 -0700 Message-ID: Subject: Re: [PATCH] mm/mmap: undo ->mmap() when arch_validate_flags() fails To: Carlos Llamas Cc: Andrew Morton , Catalin Marinas , Michal Hocko , Christian Brauner , linux-mm@kvack.org, bpf@vger.kernel.org, kernel-team@android.com, Liam Howlett , Suren Baghdasaryan , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=OWwpfDo6; spf=pass (imf24.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1664577264; a=rsa-sha256; cv=none; b=RuY1qq7whp7OfE8WqMSyEPX3o6C7CNK7w8WiM6v6VLod3RVO9TFA1Xf4CTJBQXhUMbkeLq Zq4iKdvsqEseFD0WvBYzcT5gOBq6sVr0R3zCT7p9gH0kEz0+YxvxUpNDyzoWH2nNNzq+lq o19vcoM4evbKA/m0hgonKraihgroCu0= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1664577264; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=7eVHcRj7bOTZCSoAUca2KGtBBs0g8lofOLyuo3DNi08=; b=n+k33wx8oa6z1vyck9U8NinTN9Yh16Te/OIuUtz3ulWGoH7n3iFrsuFql5YYY4Jq3TKgHK zokdr4b/VEATqeKs2FvWjW+S1e7fsBPPFkoFTBudgZvzko1EnIp4aP8a1Fv8FGs0qy/Mqj hjl74G7KNGRpDVRbsZe3AfZSnFCMbk0= Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=OWwpfDo6; spf=pass (imf24.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.218.41 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Rspam-User: X-Stat-Signature: t14n5yi8abxwrdsqpo4rq1suf8axbcud X-Rspamd-Queue-Id: 737B9180012 X-Rspamd-Server: rspam05 X-HE-Tag: 1664577264-968612 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Sep 29, 2022 at 5:51 PM Carlos Llamas wrote: > > Commit c462ac288f2c ("mm: Introduce arch_validate_flags()") added a late > check in mmap_region() to let architectures validate vm_flags. The check > needs to happen after calling ->mmap() as the flags can potentially be > modified during this callback. > > If arch_validate_flags() check fails we unmap and free the vma. However, > the error path fails to undo the ->mmap() call that previously succeeded > and depending on the specific ->mmap() implementation this translates to > reference increments, memory allocations and other operations what will > not be cleaned up. > > There are several places (mainly device drivers) where this is an issue. > However, one specific example is bpf_map_mmap() which keeps count of the > mappings in map->writecnt. The count is incremented on ->mmap() and then > decremented on vm_ops->close(). When arch_validate_flags() fails this > count is off since bpf_map_mmap_close() is never called. > > One can reproduce this issue in arm64 devices with MTE support. Here the > vm_flags are checked to only allow VM_MTE if VM_MTE_ALLOWED has been set > previously. From userspace then is enough to pass the PROT_MTE flag to > mmap() syscall to trigger the arch_validate_flags() failure. > > The following program reproduces this issue: > --- > #include > #include > #include > #include > #include > > int main(void) > { > union bpf_attr attr = { > .map_type = BPF_MAP_TYPE_ARRAY, > .key_size = sizeof(int), > .value_size = sizeof(long long), > .max_entries = 256, > .map_flags = BPF_F_MMAPABLE, > }; > int fd; > > fd = syscall(__NR_bpf, BPF_MAP_CREATE, &attr, sizeof(attr)); > mmap(NULL, 4096, PROT_WRITE | PROT_MTE, MAP_SHARED, fd, 0); > > return 0; > } > --- > > By manually adding some log statements to the vm_ops callbacks we can > confirm that when passing PROT_MTE to mmap() the map->writecnt is off > upon ->release(): > > With PROT_MTE flag: > root@debian:~# ./bpf-test > [ 111.263874] bpf_map_write_active_inc: map=9 writecnt=1 > [ 111.288763] bpf_map_release: map=9 writecnt=1 > > Without PROT_MTE flag: > root@debian:~# ./bpf-test > [ 157.816912] bpf_map_write_active_inc: map=10 writecnt=1 > [ 157.830442] bpf_map_write_active_dec: map=10 writecnt=0 > [ 157.832396] bpf_map_release: map=10 writecnt=0 > > This patch fixes the above issue by calling vm_ops->close() when the > arch_validate_flags() check fails, after this we can proceed to unmap > and free the vma on the error path. > > Fixes: c462ac288f2c ("mm: Introduce arch_validate_flags()") > Cc: Catalin Marinas > Cc: Liam Howlett > Cc: Suren Baghdasaryan > Cc: # v5.10+ > Signed-off-by: Carlos Llamas > --- Makes sense to me, open/close callbacks should be symmetrical. From BPF-side of things: Acked-by: Andrii Nakryiko > mm/mmap.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 9d780f415be3..36c08e2c78da 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1797,7 +1797,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > if (!arch_validate_flags(vma->vm_flags)) { > error = -EINVAL; > if (file) > - goto unmap_and_free_vma; > + goto close_and_free_vma; > else > goto free_vma; > } > @@ -1844,6 +1844,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > return addr; > > +close_and_free_vma: > + if (vma->vm_ops && vma->vm_ops->close) > + vma->vm_ops->close(vma); > unmap_and_free_vma: > fput(vma->vm_file); > vma->vm_file = NULL; > -- > 2.38.0.rc1.362.ged0d419d3c-goog >