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 924D8C10DCE for ; Thu, 7 Dec 2023 09:24:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1E13B6B0087; Thu, 7 Dec 2023 04:24:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 192336B0088; Thu, 7 Dec 2023 04:24:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 080B86B0089; Thu, 7 Dec 2023 04:24:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id EDF116B0087 for ; Thu, 7 Dec 2023 04:23:59 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id BA15CA014F for ; Thu, 7 Dec 2023 09:23:59 +0000 (UTC) X-FDA: 81539485398.08.2162840 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by imf28.hostedemail.com (Postfix) with ESMTP id 1908CC001C for ; Thu, 7 Dec 2023 09:23:55 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=none; spf=pass (imf28.hostedemail.com: domain of wangkefeng.wang@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1701941037; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Lm0mJYIZJ+3PndBWbznFdDAZ10agy8gtXomvvwWslTM=; b=y/WULxYO5zbcy2sliHTHIzT1BRGXZdufwq9d5Cog579U78v+tS2xEa+tKIaLsgH9EOZyBu L6G1foRz2qbx7kHtA4nlHF5pY5NH6H9hKaNLFHs0z2A/ZJcT1RzvwiY+fS6++by7A1SGyz /ZhY/3f8AiYxS7M6VWgJ+T0ca+SfqfM= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1701941037; a=rsa-sha256; cv=none; b=PLXftcsDGIPS7tDoyyaaK9Mk1P81PXgChcYZvlCp44oTRmSsgsFLVN7f24j3iTIGyGQYTW 0DuElmF5Web3Tz+xdkvBlRuLvNGfkwfHMFtxDLTl7SY3jlt4zE3pwzNPY8HmfFslzy+hQk cUrAlyCrcT5ax6+5nE76hFanNrNXPhg= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=none; spf=pass (imf28.hostedemail.com: domain of wangkefeng.wang@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=wangkefeng.wang@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com Received: from dggpemm100001.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Sm7y80mwbzShbZ; Thu, 7 Dec 2023 17:19:28 +0800 (CST) Received: from [10.174.177.243] (10.174.177.243) by dggpemm100001.china.huawei.com (7.185.36.93) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Thu, 7 Dec 2023 17:23:50 +0800 Message-ID: Date: Thu, 7 Dec 2023 17:23:49 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/4] selinux: use vma_is_initial_stack() and vma_is_initial_heap() Content-Language: en-US To: Ondrej Mosnacek CC: Paul Moore , Stephen Smalley , SElinux list , "open list:MEMORY MANAGEMENT" , David Hildenbrand , , Andrew Morton References: <20230728050043.59880-1-wangkefeng.wang@huawei.com> <20230728050043.59880-4-wangkefeng.wang@huawei.com> <79f6131f-7d69-408d-917a-f0ca6fccc5b2@huawei.com> From: Kefeng Wang In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.177.243] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To dggpemm100001.china.huawei.com (7.185.36.93) X-CFilter-Loop: Reflected X-Rspamd-Queue-Id: 1908CC001C X-Rspam-User: X-Stat-Signature: dqonujfgkfyc73a7cnsamz4q9c3ht1kw X-Rspamd-Server: rspam03 X-HE-Tag: 1701941035-386406 X-HE-Meta: U2FsdGVkX1/dJrKhNunsF+ZPJjsT87F60gehhcjWs1W3FhxyIU9i1EUaPbrMe9qX15ZWG1Fp6+8OxdgOELwpoqyI9QkZb5xUm5TQ6b7YlowXB9bsisUklDYGBFrX3yLmrl10c4I4da2PoCiP4D7KxEjWaNnUXOruAKkdqDbJqd3vpeVrHVU0aYlqUbaZP+5WbaPZ/qA/w6pf4VOH9JCvfZwuD/qpwjBd0Smf4VDgo9PwizNcGIQUOoHojlnw6HPGdt8R81O1/54gvDl5zbRI0rE/I8kWG4xV7dNs5mIz1BocR7H/6vpB1ZML33IkC95UHAN1lblfSUQFGMUfX3NtK3NbmF6Ve0aeguLh4gW0slZEnlJmhMH45KGRr5rgSnBiN/8aZVB1heQYA6VFqWITHylWC6xQkJ9buiSMg3AgL4ap+paFWpD3qVO4SIemYePYl5c5f6YKQVzf7FDMhOQYWfsyXUNpLOaiJeoOAEWUXgDV7AP8oHvrn2RApo11Co0QkKZKIx+2Fq5WV1dM0Cp+pCz0D4AMELUrc5KQ/v06IUKIh4nBDouCwiqUOUdJI3vL2jdIl9B7DAbsMzE9iBSsGxxlGTLXQKA3xZk3hIfE8XoOgAOww4dshOTLCbNxl/juX2UrvVvFD66yBUxGKC3Y/IzdcIdKpnPpB35ndbZbxhumugBnhE4rbXJuBfLUiHvHYfmmwG+jwf66Jwx/kotE1tPecbRmuFJ1C0nryNvvxv84vLljKzEjl1VnmlJ1NmN64EnHHcQOzET7YMjAfYbSpkImhxbeG47Y5l6oEe4mvlvVvcEsG3q5gLXCigOIMOftO39xfa3es2NWoTFNPedOMecWSIFfV0Kls0R9jo9v10P8pIgZYNWBXzsLVMpsUh+w4qAvbDGcNOqPVViHI/Q9SjV8lICOe8gWiVC8HGvJCikk16rKpKYzI4J43/sCkR4m0wALjy6pCW6jnxard/B Lr2KmY+X V/NPIwbaedswCUMMLJvzE2P1eDIFemCspJgPjS7MAIPL9ACmrW9DOstIm10iewQP/U8EaogcjXeSxWSdOg+H2UNnjnicmNNWMFMFjx4zJxZJe3QAirYhCail1ZJQ0gW4IFSp4AwdgYgx6t/eqnG8z3r46P8PMhi/77t7NLg+lqVTtHXN74TQ2WMwJTIBbFKaqlUiWRo0P6hDelybvijAS8QMTs9uwsJT0mLCz5eY9kwkRubQmolMZ+sRwk1KdoFOB44iq7n3ndrSwEf+k07vUtC/5i34AiJUJxuDVIaO6EAmKbFjiERQ0Z6XCudZoN+4/NpuSc9oF3LmOjhls0UBZmCnJPyQA9pPMoga+z9Gdny6EhYhB0wxYotqQF3eNlwwnNNInfOknmmCcFSMIVFVWV4rMos11nUvNOP7UWcINpXg9+is/kmzOHOw1ZKyKSPUoseTdet1hlpmCTvn2z2YokR35o8QemlbAlNSqBXeRHyVJHiQ8QsXsC4wGow== 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: List-Subscribe: List-Unsubscribe: On 2023/12/7 16:37, Ondrej Mosnacek wrote: > On Thu, Dec 7, 2023 at 5:50 AM Kefeng Wang wrote: >> >> >> >> On 2023/12/7 4:47, Paul Moore wrote: >>> On Wed, Dec 6, 2023 at 9:22 AM Ondrej Mosnacek wrote: >>>> >>>> (Restoring part of the Cc list to include more relevant lists & >>>> people... If you are lost, the original email is here: >>>> https://lore.kernel.org/selinux/20230728050043.59880-4-wangkefeng.wang@huawei.com/) >>>> >>>> On Tue, Aug 1, 2023 at 1:08 AM Paul Moore wrote: >>>>> On Mon, Jul 31, 2023 at 4:02 PM Stephen Smalley >>>>> wrote: >>>>>> On Mon, Jul 31, 2023 at 12:19 PM Paul Moore wrote: >>>>>>> >>>>>>> On Mon, Jul 31, 2023 at 10:26 AM Stephen Smalley >>>>>>> wrote: >>>>>>>> I believe this patch yields a semantic change in the SELinux execheap >>>>>>>> permission check. That said, I think the change is for the better. >>>>>>> >>>>>>> Agreed. I'm also in favor of using a helper which is maintained by >>>>>>> the VM folks over open coded logic in the SELinux code. >>>>>> >>>>>> Yes, only caveat is in theory it could trigger new execheap denials >>>>>> under existing policies. >>>>>> Trying to construct an example based on the >>>>>> selinux-testsuite/tests/mmap/mprot_heap.c example but coming up empty >>>>>> so far on something that both works and yields different results >>>>>> before and after this patch. >>>>> >>>>> My gut feeling is that this will not be an issue, but I could very >>>>> well be wrong. If it becomes a significant issue we can revert the >>>>> SELinux portion of the patch. >>>>> >>>>> Of course, if you have any luck demonstrating this with reasonable >>>>> code, that would be good input too. >>>> >>>> So, it turns out this does affect actual code. Thus far, we know about >>>> gcl [1] and wine [2]. The gcl case is easy to reproduce (just install >>>> gcl on Fedora and run gcl without arguments), so I was able to dig a >>>> bit deeper. >>>> >>>> gcl has the following relevant memory mappings as captured by gdb: >>>> Start Addr End Addr Size Offset Perms objfile >>>> 0x413000 0xf75000 0xb62000 0x3fa000 rw-p >>>> /usr/lib/gcl-2.6.14/unixport/saved_ansi_gcl >>>> 0xf75000 0xf79000 0x4000 0x0 rwxp [heap] >>>> >>>> It tries to call mprotect(0x883000, 7282688, >>>> PROT_READ|PROT_WRITE|PROT_EXEC), i.e. it tries to make the region >>>> 0x883000 - 0xf75000 executable. Before this patch it was allowed, >>>> whereas now it triggers an execheap SELinux denial. But this seems >>>> wrong - the affected region is merely adjacent to the [heap] region, >>>> it does not actually overlap with it. So even if we accept that the >>>> correct semantics is to catch any region that overlaps with the heap >>>> (before only subregions of the heap were subject to the execheap >>>> check), this corner case doesn't seem to be handled correctly by the >>>> new check (and the same bug seems to have been in fs/proc/task_mmu.c >>>> before commit 11250fd12eb8 ("mm: factor out VMA stack and heap >>>> checks")). >> >> Yes, the heap check exists for a long time, >> >> [start_brk brk] >> case1: [vm_start,vm_end] >> case2: [vm_start,vm_end] >> case3: [vm_start,vm_end] >> case4: [vm_start, vm_end] >> >> case5: [vm_start,vm_end] >> case6: [vm_start,vm_end] >> >> old check: >> vma->vm_start >= vma->vm_mm->start_brk && vma->vm_end <= vma->vm_mm->brk >> >> Only include case1， vma range must be within heap >> >> new check: >> vma->vm_start <= vma->vm_mm->brk && vma->vm_end >= vma->vm_mm->start_brk >> >> Include case1~case6, but case5(vm_end=start_brk) and case6(vm_start=brk) >> are the corner cases, gcl issue matchs the case5. >> >>>> >>>> I didn't analyze the wine case ([2]), but it may be the same situation. >>>> >>>> Unless I'm mistaken, those <= & >= in should in fact be just < & >. >> >> I support this change. >> vma->vm_start < vma->vm_mm->brk && vma->vm_end > vma->vm_mm->start_brk >> >>>> And the expression in vma_is_initial_stack() is also suspicious (but >>>> I'm not going to make any assumption on what is the intended semantics >>>> there...) >>>> >>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2252391 >>>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=2247299 >> >> Could you quickly verify after the above change? > > Yes, changing the operators as suggested fixes the gcl case. Thanks for your confirm，win[2] fixed too, right? > > BTW, the vma_is_*() helpers introduced by your patch also have wrong > indentation - the "return" lines are indented by 7 spaces instead of a > tab. If you are going to submit a fixing patch you might want to fix > that, too. Sure. will fix.