Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 16 May 2022 17:42:38 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     "Kalra, Ashish" <Ashish.Kalra@amd.com>
Cc:     Peter Gonda <pgonda@google.com>,
        "Allen, John" <John.Allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        "Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Andy Nguyen <theflow@google.com>,
        David Rientjes <rientjes@google.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH v2] crypto: ccp - Use kzalloc for sev ioctl interfaces to
 prevent kernel memory leak
Message-ID: <YoKNDoiXKGbBhuIk@google.com>
References: <20220516154512.259759-1-john.allen@amd.com>
 <CAMkAt6oUxUFtNS4W0bzu13oWMdfnzfNrphH3OqwAkmxJcXhOqw@mail.gmail.com>
 <SN6PR12MB27678261F176C5D9B5BF64EF8ECF9@SN6PR12MB2767.namprd12.prod.outlook.com>
 <CAMkAt6q7kTGS5QgZRq9xc0HaEYyntmj3GRWehr-3Sb4y2eQ=HQ@mail.gmail.com>
 <SN6PR12MB2767B4A3919E38C7F429CC2D8ECF9@SN6PR12MB2767.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SN6PR12MB2767B4A3919E38C7F429CC2D8ECF9@SN6PR12MB2767.namprd12.prod.outlook.com>
Precedence: bulk

On Mon, May 16, 2022, Kalra, Ashish wrote:
> > >Would it be safer to memset @data here to all zeros too?
> >
> > It will be, but this command/function is safe as firmware will fill in the
> > whole buffer here with the PLATFORM STATUS data retuned to the user.
> 
> > That does seem safe for now but I thought we decided it would be prudent to
> > not trust the PSPs implementation here and clear all the buffers that
> > eventually get sent to userspace?
> 
> Yes, but the issue is when the user programs a buffer size larger the one
> filled in by the firmware. In this case firmware is always going to fill up
> the whole buffer with PLATFORM_STATUS data, so it will be always be safe. The
> issue is mainly with the kernel side doing a copy_to_user() based on user
> programmed length instead of the firmware returned buffer length.

Peter's point is that it costs the kernel very little to be paranoid and not make
assumptions about whether or not the PSP will fill an entire struct as expected.

I agree it feels a bit silly since all fields are output, but on the other hand the
PSP spec just says:

  The following data structure is written to memory at STATUS_PADDR

and the data structure has several reserved fields.  I don't love assuming that the
PSP will always write zeros for the reserved fields and not do something like:

	if (rmp_initialized)
		data[3] |= IS_RMP_INIT;
	else
		data[3] &= ~IS_RMP_INIT;

Given that zeroing @data in the kernel is easy and this is not a hot patch, I
prefer the paranoid approach unless the PSP spec is much more explicit in saying
that it writes all bits and bytes on success.