Received-SPF: pass (google.com: domain of linux-kernel+bounces-99850-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240311161558.1310-1-mhklinux@outlook.com> <20240311161558.1310-3-mhklinux@outlook.com>
 <13581af9-e5f0-41ca-939f-33948b2133e7@linux.intel.com>
In-Reply-To: <13581af9-e5f0-41ca-939f-33948b2133e7@linux.intel.com>
Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Date: Mon, 11 Mar 2024 22:45:54 -0700
Message-ID: <CAC41dw9RKh1EzprEHqy+BpRbPNYiwOLvnWeauErdMLDCJdRyLA@mail.gmail.com>
Subject: Re: [PATCH v2 2/5] Drivers: hv: vmbus: Track decrypted status in vmbus_gpadl
To: mhklinux@outlook.com, rick.p.edgecombe@intel.com, kys@microsoft.com, 
	haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, 
	gregkh@linuxfoundation.org, davem@davemloft.net, edumazet@google.com, 
	kuba@kernel.org, pabeni@redhat.com, kirill.shutemov@linux.intel.com, 
	dave.hansen@linux.intel.com, linux-kernel@vger.kernel.org, 
	linux-hyperv@vger.kernel.org, netdev@vger.kernel.org, 
	linux-coco@lists.linux.dev
Cc: elena.reshetova@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Mar 11, 2024 at 10:02=E2=80=AFPM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 3/11/24 9:15 AM, mhkelley58@gmail.com wrote:
> > From: Rick Edgecombe <rick.p.edgecombe@intel.com>
> >
> > In CoCo VMs it is possible for the untrusted host to cause
> > set_memory_encrypted() or set_memory_decrypted() to fail such that an
> > error is returned and the resulting memory is shared. Callers need to
> > take care to handle these errors to avoid returning decrypted (shared)
> > memory to the page allocator, which could lead to functional or securit=
y
> > issues.
> >
> > In order to make sure callers of vmbus_establish_gpadl() and
> > vmbus_teardown_gpadl() don't return decrypted/shared pages to
> > allocators, add a field in struct vmbus_gpadl to keep track of the
> > decryption status of the buffers. This will allow the callers to
> > know if they should free or leak the pages.
> >
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> > ---
> >  drivers/hv/channel.c   | 25 +++++++++++++++++++++----
> >  include/linux/hyperv.h |  1 +
> >  2 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > index 56f7e06c673e..bb5abdcda18f 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -472,9 +472,18 @@ static int __vmbus_establish_gpadl(struct vmbus_ch=
annel *channel,
> >               (atomic_inc_return(&vmbus_connection.next_gpadl_handle) -=
 1);
> >
> >       ret =3D create_gpadl_header(type, kbuffer, size, send_offset, &ms=
ginfo);
> > -     if (ret)
> > +     if (ret) {
> > +             gpadl->decrypted =3D false;
>
> Why not set it by default at the beginning of the function?
>
> >               return ret;
> > +     }
> >
> > +     /*
> > +      * Set the "decrypted" flag to true for the set_memory_decrypted(=
)
> > +      * success case. In the failure case, the encryption state of the
> > +      * memory is unknown. Leave "decrypted" as true to ensure the
> > +      * memory will be leaked instead of going back on the free list.
> > +      */
> > +     gpadl->decrypted =3D true;
> >       ret =3D set_memory_decrypted((unsigned long)kbuffer,
> >                                  PFN_UP(size));
> >       if (ret) {
> > @@ -563,9 +572,15 @@ static int __vmbus_establish_gpadl(struct vmbus_ch=
annel *channel,
> >
> >       kfree(msginfo);
> >
> > -     if (ret)
> > -             set_memory_encrypted((unsigned long)kbuffer,
> > -                                  PFN_UP(size));
> > +     if (ret) {
> > +             /*
> > +              * If set_memory_encrypted() fails, the decrypted flag is
> > +              * left as true so the memory is leaked instead of being
> > +              * put back on the free list.
> > +              */
> > +             if (!set_memory_encrypted((unsigned long)kbuffer, PFN_UP(=
size)))
> > +                     gpadl->decrypted =3D false;
> > +     }
> >
> >       return ret;
> >  }
> > @@ -886,6 +901,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel *chan=
nel, struct vmbus_gpadl *gpad
> >       if (ret)
> >               pr_warn("Fail to set mem host visibility in GPADL teardow=
n %d.\n", ret);
>
> Will this be called only if vmbus_establish_gpad() is successful? If not,=
 you might want to skip
> set_memory_encrypted() call for decrypted =3D false case.
>
> >
> > +     gpadl->decrypted =3D ret;
> > +
>
> IMO, you can set it to false by default. Any way with non zero return, us=
er know about the
> decryption failure.

I understand this change after looking at the rest of the patches. So
please ignore the above
comment.

>
> >       return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
> > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> > index 2b00faf98017..5bac136c268c 100644
> > --- a/include/linux/hyperv.h
> > +++ b/include/linux/hyperv.h
> > @@ -812,6 +812,7 @@ struct vmbus_gpadl {
> >       u32 gpadl_handle;
> >       u32 size;
> >       void *buffer;
> > +     bool decrypted;
> >  };
> >
> >  struct vmbus_channel {
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>