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:   Jorgen Hansen <jhansen@vmware.com>
To:     Stefano Garzarella <sgarzare@redhat.com>,
        Norbert Slusarek <nslusarek@gmx.net>,
        Arnd Bergmann <arnd@kernel.org>
CC:     gregkh <gregkh@linuxfoundation.org>, Arnd Bergmann <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH RESEND] misc/vmw_vmci: bail out earlier on too big queue
 allocation
Thread-Topic: [PATCH RESEND] misc/vmw_vmci: bail out earlier on too big queue
 allocation
Thread-Index: AQHWwdvBx3YmmaRjgU6QHAKkQ+OYDKniutsAgADm2QCACBbh0A==
Date:   Mon, 7 Dec 2020 14:40:46 +0000
Message-ID: <DM5PR05MB3452D9FA3B2B1FB066B55BE1DACE0@DM5PR05MB3452.namprd05.prod.outlook.com>
References: <trinity-56749cfc-536b-45e2-8e6f-621754c07824-1606165260518@3c-app-gmx-bs09>
 <CAK8P3a2dq8VtYARPS9r9aeJipA3hD9M01wmEJ8LuZT1_Taav3w@mail.gmail.com>
 <CAGxU2F7-suw3PT0GKw68AHTaOzUOgbBRCZmNOZd+MH+Y+q8EyA@mail.gmail.com>
In-Reply-To: <CAGxU2F7-suw3PT0GKw68AHTaOzUOgbBRCZmNOZd+MH+Y+q8EyA@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5d6519ca-4c99-4e58-fd99-08d89abe159a
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Dec 2020 14:40:46.8508
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8zgz2TtOYLnOUZuZyQKBEeN53NtlStEpmdEE11Y6SqMFmXj2+ttZ7t4vq5oU9iwfD9kkfwfzlrbH9wKb+M6Tpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR05MB7207
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Stefano Garzarella <sgarzare@redhat.com>=0A=
Sent: Wednesday, December 2, 2020 2:06 AM=0A=
> =0A=
> On Tue, Dec 1, 2020 at 9:21 PM Arnd Bergmann <arnd@kernel.org> wrote:=0A=
> >=0A=
> > On Mon, Nov 23, 2020 at 10:01 PM Norbert Slusarek <nslusarek@gmx.net>=
=0A=
> wrote:=0A=
> > >=0A=
> > > From: Norbert Slusarek <nslusarek@gmx.net>=0A=
> > > Date: Mon, 23 Nov 2020 21:53:41 +0100=0A=
> > > Subject: [PATCH RESEND] misc/vmw_vmci: bail out earlier on too big=0A=
> > > queue allocation=0A=
> > >=0A=
> > > For the allocation of a queue pair in qp_host_alloc_queue() an=0A=
> > > arbitrary value can be passed for produce_size, which can lead to=0A=
> > > miscalculation of memory we'd like to allocate in one take. A=0A=
> > > warning is triggered at __alloc_pages_nodemask() in=0A=
> > > mm/page_alloc.c:4737 which aborts the false allocation, but it result=
s in a=0A=
> VMware machine freezing for an infinite time.=0A=
> > >=0A=
> > > Signed-off-by: Norbert Slusarek <nslusarek@gmx.net>=0A=
> >=0A=
> > Thank you for sending a patch with such a detailed analysis and even=0A=
> > including a test case!=0A=
> =0A=
> Yeah agree, very good details!=0A=
=0A=
Yes, thanks a lot for pointing this out.=0A=
=0A=
> >=0A=
> > > diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c=0A=
> > > b/drivers/misc/vmw_vmci/vmci_queue_pair.c=0A=
> > > index c49065887e8f..997ff32475b2 100644=0A=
> > > --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c=0A=
> > > +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c=0A=
> > > @@ -526,6 +526,7 @@ static struct vmci_queue=0A=
> *qp_host_alloc_queue(u64 size)=0A=
> > >         struct vmci_queue *queue;=0A=
> > >         size_t queue_page_size;=0A=
> > >         u64 num_pages;=0A=
> > > +       unsigned int order;=0A=
> > >         const size_t queue_size =3D sizeof(*queue) +=0A=
> > > sizeof(*(queue->kernel_if));=0A=
> > >=0A=
> > >         if (size > SIZE_MAX - PAGE_SIZE) @@ -537,6 +538,10 @@ static=
=0A=
> > > struct vmci_queue *qp_host_alloc_queue(u64 size)=0A=
> > >=0A=
> > >         queue_page_size =3D num_pages *=0A=
> > > sizeof(*queue->kernel_if->u.h.page);=0A=
> > >=0A=
> > > +       order =3D get_order(queue_size + queue_page_size);=0A=
> > > +       if (order >=3D MAX_ORDER)=0A=
> > > +               return NULL;=0A=
> > > +=0A=
> > >         queue =3D kzalloc(queue_size + queue_page_size, GFP_KERNEL);=
=0A=
> >=0A=
> > Calling kzalloc() with that user-provided size may still not be a=0A=
> > great idea, and MAX_ORDER is probably more than anyone ever needs=0A=
> here=0A=
> > (I don't know what the interface is needed for, but usually it is).=0A=
> >=0A=
> > If there is a reasonable upper bound smaller than MAX_ORDER, I would=0A=
> > suggest using that. It might also be helpful to use=0A=
> > kvzalloc()/kvfree() in this case, which can return an arbitrary number=
=0A=
> > of pages and suffers less from fragmentation.=0A=
> =0A=
> I don't know well vmw_vmci, but I took a brief look, and I saw that there=
 is a=0A=
> macro (VMCI_MAX_GUEST_QP_MEMORY) used in vmci_qpair_alloc(), I'm=0A=
> not sure if we can use the same macro, but maybe something similar.=0A=
> =0A=
> Honestly I don't know where that limit comes from, maybe it was chosen as=
=0A=
> an arbitrary and reasonable value but I suggest to check if we can reuse =
the=0A=
> same macro here with some adjustments.=0A=
> I think in some way that limit is related to the max memory that the host=
=0A=
> should allocate.=0A=
> =0A=
> @Jorgen any thought?=0A=
=0A=
The VMCI_MAX_GUEST_QP_MEMORY limit is the limit for the total amount of gue=
st memory that can be used for VMCI queue pairs with the current revision o=
f the VMCI virtual device, so it is the upper limit on the size of a single=
 queue pair as well. In this function, the size parameter indicates the des=
ired queue size of one of the queues in the queue pair, so they should be b=
ounded by this as well. The appropriate check in the beginning of the funct=
ion would be=0A=
=0A=
 if (size > min(VMCI_MAX_GUEST_QP_MEMORY, SIZE_MAX - PAGE_SIZE))=0A=
    return NULL;=0A=
=0A=
Since VMCI_MAX_GUEST_QP_MEMORY is 128MB, the actual kzalloc argument should=
 be well below the limit imposed by MAX_ORDER, with the modification of the=
 above check.=0A=
=0A=
That this unchecked value can occur here at all is due to another bug, wher=
e the queue sizes aren't validated against the VMCI_MAX_GUEST_QP_MEMORY lim=
it on the vmci_qp_broker_alloc callpath. We'll fix that and we can update t=
he above check as well in the same change.=0A=
=0A=
Thanks,=0A=
J=F8rgen=0A=
=0A=
=0A=