Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201123063835.18981-1-miles.chen@mediatek.com>
In-Reply-To: <20201123063835.18981-1-miles.chen@mediatek.com>
From:   Catalin Marinas <catalin.marinas@arm.com>
Date:   Thu, 26 Nov 2020 11:10:09 +0000
Message-ID: <CAHkRjk7xGoU=KBeFE4gy=yxkLhvHqz2A1JyCBKF8dhjJNDD=zA@mail.gmail.com>
Subject: Re: [RESEND PATCH v1] proc: use untagged_addr() for pagemap_read addresses
To:     Miles Chen <miles.chen@mediatek.com>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        wsd_upstream@mediatek.com, andreyknvl@google.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Miles,

Could you please cc me and Andrey Konovalov on future versions of this
patch (if any)?

On Mon, 23 Nov 2020 at 08:47, Miles Chen <miles.chen@mediatek.com> wrote:
> When we try to visit the pagemap of a tagged userspace pointer, we find
> that the start_vaddr is not correct because of the tag.
> To fix it, we should untag the usespace pointers in pagemap_read().
>
> I tested with 5.10-rc4 and the issue remains.
>
> My test code is baed on [1]:
>
> A userspace pointer which has been tagged by 0xb4: 0xb400007662f541c8
>
> === userspace program ===
>
> uint64 OsLayer::VirtualToPhysical(void *vaddr) {
>         uint64 frame, paddr, pfnmask, pagemask;
>         int pagesize = sysconf(_SC_PAGESIZE);
>         off64_t off = ((uintptr_t)vaddr) / pagesize * 8; // off = 0xb400007662f541c8 / pagesize * 8 = 0x5a00003b317aa0

Arguably, that's a user-space bug since tagged file offsets were never
supported. In this case it's not even a tag at bit 56 as per the arm64
tagged address ABI but rather down to bit 47. You could say that the
problem is caused by the C library (malloc()) or whoever created the
tagged vaddr and passed it to this function. It's not a kernel
regression as we've never supported it.

Now, pagemap is a special case where the offset is usually not
generated as a classic file offset but rather derived by shifting a
user virtual address. I guess we can make a concession for pagemap
(only) and allow such offset with the tag at bit (56 - PAGE_SHIFT +
3).

Please fix the patch as per Eric's suggestion on avoiding the
overflow. You should also add a Cc: stable v5.4- as that's when we
enabled the tagged address ABI on arm64 and when it's more likely for
the C library/malloc() to start generating such pointers.

If the problem is only limited to this test, I'd rather fix the user
but I can't tell how widespread the /proc/pid/pagemap usage is.

Thanks.

-- 
Catalin