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;
From:   Liam Howlett <liam.howlett@oracle.com>
To:     David Hildenbrand <david@redhat.com>
CC:     Niklas Schnelle <schnelle@linux.ibm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-s390@vger.kernel.org" <linux-s390@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
Subject: Re: [PATCH RFC 6/9] s390/pci_mmio: fully validate the VMA before
 calling follow_pte()
Thread-Topic: [PATCH RFC 6/9] s390/pci_mmio: fully validate the VMA before
 calling follow_pte()
Thread-Index: AQHXpYt94rWRlMpIgEO//XQ8blRXe6uc7v8AgAARCACAAFCfgA==
Date:   Fri, 10 Sep 2021 14:12:28 +0000
Message-ID: <20210910141221.fuimjijydw57vxjz@revolver>
References: <20210909145945.12192-1-david@redhat.com>
 <20210909145945.12192-7-david@redhat.com>
 <82d683ec361245e1879b3f14492cdd5c41957e52.camel@linux.ibm.com>
 <d9ec2387-2645-796e-af47-26f22516f7fa@redhat.com>
In-Reply-To: <d9ec2387-2645-796e-af47-26f22516f7fa@redhat.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cyVzeeby8izmfSTGAfw29gGCyJ/ZDRIXz8a0Y4Nr8R7mpIkE9+b42nELCBjv?=
 =?us-ascii?Q?jL8d6OS4/QhETj3ykW4Ud3i19CVLS1v9LfvSm6bANS3RPK+SevLWrccDCeCB?=
 =?us-ascii?Q?B97Ov4k7C+i0IhG3U7wWWPV/C9Nhc3xaKh77oW3Jffek0BaMs2/6/4jNzgg4?=
 =?us-ascii?Q?siQs+c52GGuWueNtytsWIvJiFWnYY0fb36hNWsfpxXvsuiCuqEX5/DfTL4PP?=
 =?us-ascii?Q?hIN6uf+yC1oM494ACLuWGavsgtsRmlK+dLVC/n/Btn5c2f3MWRBi2QFfO6LO?=
 =?us-ascii?Q?wopc5OmZ0lYnVeh01+n1bd9+o3hw18NKq76Zi6u8Lw5HKlDEMXFAzErA+KoD?=
 =?us-ascii?Q?d//e4Wypib8ktU405Q8Zyo9FhC90UFVZRSwaMAJ5IdhbCzocehe5aqUjvS7p?=
 =?us-ascii?Q?B7vX3iP5vAdSSgln1m7uiwOFH+Jb/bWOfnRKWJfzCRl++IE69L8moz6CwJP9?=
 =?us-ascii?Q?bwxAynH440zqdHHXK2xa/r3V+dJ4nB+t92m6hHJuC2kZtx2l/tOnDx4Ud1CI?=
 =?us-ascii?Q?HrSqKZlgir5z6GoNgw4Yy1IzcbVlUERdz/epM4XerwnQxyWzGM/Df2al2Wpt?=
 =?us-ascii?Q?Ipr16D3Z/UGveBNEzkma3PUW0Ux7IEyaI5nOOkAtF/Jc9qN9LuwkxfMGw+Nj?=
 =?us-ascii?Q?PcaYvI+p7Og4UxB1Wkf84QFpshcBIhZrw9aiqfpVgGGaIezZUnPrhM7YAtNr?=
 =?us-ascii?Q?XwkjtdWMK6Et4Z6oxga0QMK81S1SHDlfNZjHfKMffNenZySmuecWpVlfHbZT?=
 =?us-ascii?Q?vO2+CB3XxP5pQiOCY2ZKdqYs0bArq3G2en/pl0z6EGY9ltES0c4yrz0JaRqc?=
 =?us-ascii?Q?jfkCMtYxdLawRbRNbXpATft12EsQ44hq3OtPyy+r7ydgwE3RGDh/hZaOuq2S?=
 =?us-ascii?Q?uwRSLCB0FM75ROyKqaOnhWQmVNXM8zzBO9nMoNd53Go1J4oDOgPvZBkqqAh0?=
 =?us-ascii?Q?+3xQpg79P1UeZbTn3avnPWEE7RsMCVFobs2ckX8QMeiAaJN7OjyQRNMYeKOJ?=
 =?us-ascii?Q?PQNFBsL6KJrglkVAh/vOHErZveTSE1OCJcb464zu59ilmNjULnfC+6X/KmQV?=
 =?us-ascii?Q?Gr/MzmBwnscHTW+wiKEDJvy/ctytxRVkbX6mynMpYp4s08DlqcL7dJTwsjCP?=
 =?us-ascii?Q?UQGwWMEzIn27itV4OMBMK3ORM28IZzpGGi7bBT0bUrx6ZM/9ldvMSZ3vX7JB?=
 =?us-ascii?Q?AqsK5Kx1kC5s6SiVKtindR0n4tpClhRo/8BSaPzinTYzrGZJs3t/LEjbaxnP?=
 =?us-ascii?Q?Hf6M9lJecs3VgZn1BE2ajyB/VynvDsA+DX+jNr+HVXeFzeCS1/Fkfmcs6wFT?=
 =?us-ascii?Q?F6+uRPo2KZOV/wQvcvqeWGaH?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3FA59300422AFC4CB78E15D43825E10E@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR10MB4380.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b25e6214-2186-4fe7-7dad-08d9746505f8
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Sep 2021 14:12:28.8852
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: rTm8wxXp7fqGK15lJumWYjmG91UKbZggIswyotdInbL1VjfY+UqVOAqqFUlKHjVYqD3YJac4XBLy8CkCnUZ4uQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3337
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10102 signatures=668682
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0 mlxscore=0
 suspectscore=0 spamscore=0 adultscore=0 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2109030001
 definitions=main-2109100081
X-Proofpoint-ORIG-GUID: 5uq_wtdkdpaWfEvhMtQWEyOzhz32c-o6
X-Proofpoint-GUID: 5uq_wtdkdpaWfEvhMtQWEyOzhz32c-o6
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* David Hildenbrand <david@redhat.com> [210910 05:23]:
> On 10.09.21 10:22, Niklas Schnelle wrote:
> > On Thu, 2021-09-09 at 16:59 +0200, David Hildenbrand wrote:
> > > We should not walk/touch page tables outside of VMA boundaries when
> > > holding only the mmap sem in read mode. Evil user space can modify th=
e
> > > VMA layout just before this function runs and e.g., trigger races wit=
h
> > > page table removal code since commit dd2283f2605e ("mm: mmap: zap pag=
es
> > > with read mmap_sem in munmap").
> > >=20
> > > find_vma() does not check if the address is >=3D the VMA start addres=
s;
> > > use vma_lookup() instead.
> > >=20
> > > Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munma=
p")
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > ---
> > >   arch/s390/pci/pci_mmio.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >=20
> > > diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
> > > index ae683aa623ac..c5b35ea129cf 100644
> > > --- a/arch/s390/pci/pci_mmio.c
> > > +++ b/arch/s390/pci/pci_mmio.c
> > > @@ -159,7 +159,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_write, unsigned lon=
g, mmio_addr,
> > >   	mmap_read_lock(current->mm);
> > >   	ret =3D -EINVAL;
> > > -	vma =3D find_vma(current->mm, mmio_addr);
> > > +	vma =3D vma_lookup(current->mm, mmio_addr);
> > >   	if (!vma)
> > >   		goto out_unlock_mmap;
> > >   	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
> > > @@ -298,7 +298,7 @@ SYSCALL_DEFINE3(s390_pci_mmio_read, unsigned long=
, mmio_addr,
> > >   	mmap_read_lock(current->mm);
> > >   	ret =3D -EINVAL;
> > > -	vma =3D find_vma(current->mm, mmio_addr);
> > > +	vma =3D vma_lookup(current->mm, mmio_addr);
> > >   	if (!vma)
> > >   		goto out_unlock_mmap;
> > >   	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
> >=20
> > Oh wow great find thanks! If I may say so these are not great function
> > names. Looking at the code vma_lookup() is inded find_vma() plus the
> > check that the looked up address is indeed inside the vma.
> >=20
>=20
> IIRC, vma_lookup() was introduced fairly recently. Before that, this
> additional check was open coded (and still are in some instances). It's
> confusing, I agree.

This confusion is why I introduced vma_lookup().  My hope is to reduce
the users of find_vma() to only those that actually need the added
functionality, which are mostly in the mm code.

>=20
> > I think this is pretty independent of the rest of the patches, so do
> > you want me to apply this patch independently or do you want to wait
> > for the others?
>=20
> Sure, please go ahead and apply independently. It'd be great if you could
> give it a quick sanity test, although I don't expect surprises --
> unfortunately, the environment I have easily at hand is not very well sui=
ted
> (#cpu, #mem, #disk ...) for anything that exceeds basic compile tests (an=
d
> even cross-compiling is significantly faster ...).
>=20
> >=20
> > In any case:
> >=20
> > Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> >=20
>=20
> Thanks!
>=20
> --=20
> Thanks,
>=20
> David / dhildenb
>=20
>=20


Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>=