| Message ID | 20240409151908.541589-1-sebastianene@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Add support for FFA_PARTITION_INFO_GET | expand |
Hi Seb, On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote: > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor > and copy the response message back to the host buffers. Save the > returned FF-A version as we will need it later to interpret the response > from the TEE. > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > --- > arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > index 320f2eaa14a9..72fc365bc7a8 100644 > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { > */ > static struct kvm_ffa_buffers hyp_buffers; > static struct kvm_ffa_buffers host_buffers; > +static u32 ffa_version; > > static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > { > @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, > return true; > } > > +static void do_ffa_part_get(struct arm_smccc_res *res, > + struct kvm_cpu_context *ctxt) > +{ > + DECLARE_REG(u32, uuid0, ctxt, 1); > + DECLARE_REG(u32, uuid1, ctxt, 2); > + DECLARE_REG(u32, uuid2, ctxt, 3); > + DECLARE_REG(u32, uuid3, ctxt, 4); > + DECLARE_REG(u32, flags, ctxt, 5); > + u32 off, count, sz, buf_sz; > + > + hyp_spin_lock(&host_buffers.lock); > + if (!host_buffers.rx) { > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > + goto out_unlock; > + } > + > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > + uuid2, uuid3, flags, 0, 0, > + res); > + > + if (res->a0 != FFA_SUCCESS) > + goto out_unlock; > + > + count = res->a2; > + if (!count) > + goto out_unlock; Looking at the table 13.34, it seems what's in "count" depends on the flag. Shouldn't we check its value, and only memcpy into the host buffers if the flag is 0? > + > + if (ffa_version > FFA_VERSION_1_0) { > + buf_sz = sz = res->a3; > + if (sz > sizeof(struct ffa_partition_info)) > + buf_sz = sizeof(struct ffa_partition_info); What are you trying to protect against here? We have to trust EL3 anyway, (as other functions do). The WARN() could be kept though to make sure we won't overflow our buffer. But it could be transformed into an error? FFA_RET_ABORTED? > + } else { > + /* FFA_VERSION_1_0 lacks the size in the response */ > + buf_sz = sz = 8; > + } > + > + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE); > + for (off = 0; off < count * sz; off += sz) > + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz); > +out_unlock: > + hyp_spin_unlock(&host_buffers.lock); > +} > + > bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > { > struct arm_smccc_res res; > @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > case FFA_MEM_FRAG_TX: > do_ffa_mem_frag_tx(&res, host_ctxt); > goto out_handled; > + case FFA_PARTITION_INFO_GET: > + do_ffa_part_get(&res, host_ctxt); > + break; > } > > if (ffa_call_supported(func_id)) > @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages) > if (FFA_MAJOR_VERSION(res.a0) != 1) > return -EOPNOTSUPP; > > + ffa_version = res.a0; > + > arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res); > if (res.a0 != FFA_SUCCESS) > return -EOPNOTSUPP; > -- > 2.44.0.478.gd926399ef9-goog >
On Tue, Apr 09, 2024 at 05:15:20PM +0100, Vincent Donnefort wrote: > Hi Seb, > > On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote: > > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor > > and copy the response message back to the host buffers. Save the > > returned FF-A version as we will need it later to interpret the response > > from the TEE. > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > index 320f2eaa14a9..72fc365bc7a8 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { > > */ > > static struct kvm_ffa_buffers hyp_buffers; > > static struct kvm_ffa_buffers host_buffers; > > +static u32 ffa_version; > > > > static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > > { > > @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, > > return true; > > } > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > + struct kvm_cpu_context *ctxt) > > +{ > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > + DECLARE_REG(u32, flags, ctxt, 5); > > + u32 off, count, sz, buf_sz; > > + > > + hyp_spin_lock(&host_buffers.lock); > > + if (!host_buffers.rx) { > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > + goto out_unlock; > > + } > > + > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > + uuid2, uuid3, flags, 0, 0, > > + res); > > + > > + if (res->a0 != FFA_SUCCESS) > > + goto out_unlock; > > + > > + count = res->a2; > > + if (!count) > > + goto out_unlock; > > Looking at the table 13.34, it seems what's in "count" depends on the flag. > Shouldn't we check its value, and only memcpy into the host buffers if the flag > is 0? > When the flag is `1` the count referes to the number of partitions deployed. In both cases we have to copy something unless count == 0. > > + > > + if (ffa_version > FFA_VERSION_1_0) { > > + buf_sz = sz = res->a3; > > + if (sz > sizeof(struct ffa_partition_info)) > > + buf_sz = sizeof(struct ffa_partition_info); > > What are you trying to protect against here? We have to trust EL3 anyway, (as > other functions do). > > The WARN() could be kept though to make sure we won't overflow our buffer. But > it could be transformed into an error? FFA_RET_ABORTED? > > I think we can keep it as a WARN_ON because it is not expected to have a return code of FFA_SUCCESS but the buffer to be overflown. The TEE is expected to return NO_MEMORY in w2 if the results cannot fit in the RX buffer. Thanks, Seb > > + } else { > > + /* FFA_VERSION_1_0 lacks the size in the response */ > > + buf_sz = sz = 8; > > + } > > + > > + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE); > > + for (off = 0; off < count * sz; off += sz) > > + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz); > > +out_unlock: > > + hyp_spin_unlock(&host_buffers.lock); > > +} > > + > > bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > > { > > struct arm_smccc_res res; > > @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > > case FFA_MEM_FRAG_TX: > > do_ffa_mem_frag_tx(&res, host_ctxt); > > goto out_handled; > > + case FFA_PARTITION_INFO_GET: > > + do_ffa_part_get(&res, host_ctxt); > > + break; > > } > > > > if (ffa_call_supported(func_id)) > > @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages) > > if (FFA_MAJOR_VERSION(res.a0) != 1) > > return -EOPNOTSUPP; > > > > + ffa_version = res.a0; > > + > > arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res); > > if (res.a0 != FFA_SUCCESS) > > return -EOPNOTSUPP; > > -- > > 2.44.0.478.gd926399ef9-goog > >
[...] > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > > + struct kvm_cpu_context *ctxt) > > > +{ > > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > > + DECLARE_REG(u32, flags, ctxt, 5); > > > + u32 off, count, sz, buf_sz; > > > + > > > + hyp_spin_lock(&host_buffers.lock); > > > + if (!host_buffers.rx) { > > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > > + goto out_unlock; > > > + } > > > + > > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > > + uuid2, uuid3, flags, 0, 0, > > > + res); > > > + > > > + if (res->a0 != FFA_SUCCESS) > > > + goto out_unlock; > > > + > > > + count = res->a2; > > > + if (!count) > > > + goto out_unlock; > > > > Looking at the table 13.34, it seems what's in "count" depends on the flag. > > Shouldn't we check its value, and only memcpy into the host buffers if the flag > > is 0? > > > > When the flag is `1` the count referes to the number of partitions > deployed. In both cases we have to copy something unless count == 0. I see "Return the count of partitions deployed in the system corresponding to the specified UUID in w2" Which I believe means nothing has been copied in the buffer? > > > > + > > > + if (ffa_version > FFA_VERSION_1_0) { > > > + buf_sz = sz = res->a3; > > > + if (sz > sizeof(struct ffa_partition_info)) > > > + buf_sz = sizeof(struct ffa_partition_info); > > > > What are you trying to protect against here? We have to trust EL3 anyway, (as > > other functions do). > > > > The WARN() could be kept though to make sure we won't overflow our buffer. But > > it could be transformed into an error? FFA_RET_ABORTED? > > > > > > I think we can keep it as a WARN_ON because it is not expected to have > a return code of FFA_SUCCESS but the buffer to be overflown. The TEE is > expected to return NO_MEMORY in w2 if the results cannot fit in the RX > buffer. WARN() is crashing the hypervisor. It'd be a shame here as we can easily recover by just sending an error back to the caller.
On Wed, Apr 10, 2024 at 10:53:31AM +0100, Vincent Donnefort wrote: > [...] > > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > > > + struct kvm_cpu_context *ctxt) > > > > +{ > > > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > > > + DECLARE_REG(u32, flags, ctxt, 5); > > > > + u32 off, count, sz, buf_sz; > > > > + > > > > + hyp_spin_lock(&host_buffers.lock); > > > > + if (!host_buffers.rx) { > > > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > > > + goto out_unlock; > > > > + } > > > > + > > > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > > > + uuid2, uuid3, flags, 0, 0, > > > > + res); > > > > + > > > > + if (res->a0 != FFA_SUCCESS) > > > > + goto out_unlock; > > > > + > > > > + count = res->a2; > > > > + if (!count) > > > > + goto out_unlock; > > > > > > Looking at the table 13.34, it seems what's in "count" depends on the flag. > > > Shouldn't we check its value, and only memcpy into the host buffers if the flag > > > is 0? > > > > > > > When the flag is `1` the count referes to the number of partitions > > deployed. In both cases we have to copy something unless count == 0. > > I see "Return the count of partitions deployed in the system corresponding to > the specified UUID in w2" > > Which I believe means nothing has been copied in the buffer? > When the flag in w5 is 1 the size argument stored in w3 will be zero and the loop will not be executed, so nothing will be copied to the host buffers. > > > > > > + > > > > + if (ffa_version > FFA_VERSION_1_0) { > > > > + buf_sz = sz = res->a3; > > > > + if (sz > sizeof(struct ffa_partition_info)) > > > > + buf_sz = sizeof(struct ffa_partition_info); > > > > > > What are you trying to protect against here? We have to trust EL3 anyway, (as > > > other functions do). > > > > > > The WARN() could be kept though to make sure we won't overflow our buffer. But > > > it could be transformed into an error? FFA_RET_ABORTED? > > > > > > > > > > I think we can keep it as a WARN_ON because it is not expected to have > > a return code of FFA_SUCCESS but the buffer to be overflown. The TEE is > > expected to return NO_MEMORY in w2 if the results cannot fit in the RX > > buffer. > > WARN() is crashing the hypervisor. It'd be a shame here as we can easily recover > by just sending an error back to the caller. I agree with you but this is not expected to happen unless TZ messes up something/is not complaint with the spec, in which case I would like to catch this.
On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote: > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor > and copy the response message back to the host buffers. Save the > returned FF-A version as we will need it later to interpret the response > from the TEE. > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > --- > arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > index 320f2eaa14a9..72fc365bc7a8 100644 > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { > */ > static struct kvm_ffa_buffers hyp_buffers; > static struct kvm_ffa_buffers host_buffers; > +static u32 ffa_version; > > static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > { > @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, > return true; > } > > +static void do_ffa_part_get(struct arm_smccc_res *res, > + struct kvm_cpu_context *ctxt) > +{ > + DECLARE_REG(u32, uuid0, ctxt, 1); > + DECLARE_REG(u32, uuid1, ctxt, 2); > + DECLARE_REG(u32, uuid2, ctxt, 3); > + DECLARE_REG(u32, uuid3, ctxt, 4); > + DECLARE_REG(u32, flags, ctxt, 5); > + u32 off, count, sz, buf_sz; > + > + hyp_spin_lock(&host_buffers.lock); > + if (!host_buffers.rx) { > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > + goto out_unlock; > + } > + > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > + uuid2, uuid3, flags, 0, 0, > + res); > + > + if (res->a0 != FFA_SUCCESS) > + goto out_unlock; > + > + count = res->a2; > + if (!count) > + goto out_unlock; > + > + if (ffa_version > FFA_VERSION_1_0) { > + buf_sz = sz = res->a3; > + if (sz > sizeof(struct ffa_partition_info)) > + buf_sz = sizeof(struct ffa_partition_info); We are copying buf_sz but (correctly ?) returning res->a3 to the caller, which is allowed to expect res->a3 bytes to be filled since that's what firmware reported. Technically this is not a problem at present, because the caller (ie the FF-A driver) and the hypervisor rely on the same descriptor structures (and buf_sz can't be != sizeof(struct ffa_partition_info), anything else is a bug as we stand); they must be kept in sync though as the firmware version changes (*if* there are changes in the partition descriptor - eg fields are added). An option would consist in just copying res->a3 bytes as firmware reports (obviously keeping the RX buffer boundary checks for the memcpy). It is just a heads-up because I noticed it, no more, I will let Sudeep comment on this since he knows better. Lorenzo > + } else { > + /* FFA_VERSION_1_0 lacks the size in the response */ > + buf_sz = sz = 8; > + } > + > + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE); > + for (off = 0; off < count * sz; off += sz) > + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz); > +out_unlock: > + hyp_spin_unlock(&host_buffers.lock); > +} > + > bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > { > struct arm_smccc_res res; > @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > case FFA_MEM_FRAG_TX: > do_ffa_mem_frag_tx(&res, host_ctxt); > goto out_handled; > + case FFA_PARTITION_INFO_GET: > + do_ffa_part_get(&res, host_ctxt); > + break; > } > > if (ffa_call_supported(func_id)) > @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages) > if (FFA_MAJOR_VERSION(res.a0) != 1) > return -EOPNOTSUPP; > > + ffa_version = res.a0; > + > arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res); > if (res.a0 != FFA_SUCCESS) > return -EOPNOTSUPP; > -- > 2.44.0.478.gd926399ef9-goog >
On Wed, Apr 10, 2024 at 10:18:18AM +0000, Sebastian Ene wrote: > On Wed, Apr 10, 2024 at 10:53:31AM +0100, Vincent Donnefort wrote: > > [...] > > > > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > > > > + struct kvm_cpu_context *ctxt) > > > > > +{ > > > > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > > > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > > > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > > > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > > > > + DECLARE_REG(u32, flags, ctxt, 5); > > > > > + u32 off, count, sz, buf_sz; > > > > > + > > > > > + hyp_spin_lock(&host_buffers.lock); > > > > > + if (!host_buffers.rx) { > > > > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > > > > + goto out_unlock; > > > > > + } > > > > > + > > > > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > > > > + uuid2, uuid3, flags, 0, 0, > > > > > + res); > > > > > + > > > > > + if (res->a0 != FFA_SUCCESS) > > > > > + goto out_unlock; > > > > > + > > > > > + count = res->a2; > > > > > + if (!count) > > > > > + goto out_unlock; > > > > > > > > Looking at the table 13.34, it seems what's in "count" depends on the flag. > > > > Shouldn't we check its value, and only memcpy into the host buffers if the flag > > > > is 0? > > > > > > > > > > When the flag is `1` the count referes to the number of partitions > > > deployed. In both cases we have to copy something unless count == 0. > > > > I see "Return the count of partitions deployed in the system corresponding to > > the specified UUID in w2" > > > > Which I believe means nothing has been copied in the buffer? > > > > When the flag in w5 is 1 the size argument stored in w3 will be zero and > the loop will not be executed, so nothing will be copied to the host > buffers. Ha right, all good here then. > > > > > > > > > + > > > > > + if (ffa_version > FFA_VERSION_1_0) { > > > > > + buf_sz = sz = res->a3; > > > > > + if (sz > sizeof(struct ffa_partition_info)) > > > > > + buf_sz = sizeof(struct ffa_partition_info); > > > > > > > > What are you trying to protect against here? We have to trust EL3 anyway, (as > > > > other functions do). > > > > > > > > The WARN() could be kept though to make sure we won't overflow our buffer. But > > > > it could be transformed into an error? FFA_RET_ABORTED? > > > > > > > > > > > > > > I think we can keep it as a WARN_ON because it is not expected to have > > > a return code of FFA_SUCCESS but the buffer to be overflown. The TEE is > > > expected to return NO_MEMORY in w2 if the results cannot fit in the RX > > > buffer. > > > > WARN() is crashing the hypervisor. It'd be a shame here as we can easily recover > > by just sending an error back to the caller. > > I agree with you but this is not expected to happen unless TZ messes up > something/is not complaint with the spec, in which case I would like to > catch this. Hum, still I don't see the point in crashing anything here, nothing is compromised. The driver can then decide what to do based on that reported failure.
On Wed, Apr 10, 2024 at 04:57:19PM +0200, Lorenzo Pieralisi wrote: > On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote: > > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor > > and copy the response message back to the host buffers. Save the > > returned FF-A version as we will need it later to interpret the response > > from the TEE. > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > index 320f2eaa14a9..72fc365bc7a8 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { > > */ > > static struct kvm_ffa_buffers hyp_buffers; > > static struct kvm_ffa_buffers host_buffers; > > +static u32 ffa_version; > > > > static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > > { > > @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, > > return true; > > } > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > + struct kvm_cpu_context *ctxt) > > +{ > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > + DECLARE_REG(u32, flags, ctxt, 5); > > + u32 off, count, sz, buf_sz; > > + > > + hyp_spin_lock(&host_buffers.lock); > > + if (!host_buffers.rx) { > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > + goto out_unlock; > > + } > > + > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > + uuid2, uuid3, flags, 0, 0, > > + res); > > + > > + if (res->a0 != FFA_SUCCESS) > > + goto out_unlock; > > + > > + count = res->a2; > > + if (!count) > > + goto out_unlock; > > + > > + if (ffa_version > FFA_VERSION_1_0) { > > + buf_sz = sz = res->a3; > > + if (sz > sizeof(struct ffa_partition_info)) > > + buf_sz = sizeof(struct ffa_partition_info); Hello Lorenzo, > > We are copying buf_sz but (correctly ?) returning res->a3 to the caller, > which is allowed to expect res->a3 bytes to be filled since that's what > firmware reported. > > Technically this is not a problem at present, because the caller > (ie the FF-A driver) and the hypervisor rely on the same descriptor > structures (and buf_sz can't be != sizeof(struct ffa_partition_info), > anything else is a bug as we stand); they must be kept in sync though as > the firmware version changes (*if* there are changes in the partition > descriptor - eg fields are added). > > An option would consist in just copying res->a3 bytes as firmware reports > (obviously keeping the RX buffer boundary checks for the memcpy). > Ack, let me fix this an spin up a new version. > It is just a heads-up because I noticed it, no more, I will let Sudeep > comment on this since he knows better. > > Lorenzo > Thanks for having a look, Seb > > + } else { > > + /* FFA_VERSION_1_0 lacks the size in the response */ > > + buf_sz = sz = 8; > > + } > > + > > + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE); > > + for (off = 0; off < count * sz; off += sz) > > + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz); > > +out_unlock: > > + hyp_spin_unlock(&host_buffers.lock); > > +} > > + > > bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > > { > > struct arm_smccc_res res; > > @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) > > case FFA_MEM_FRAG_TX: > > do_ffa_mem_frag_tx(&res, host_ctxt); > > goto out_handled; > > + case FFA_PARTITION_INFO_GET: > > + do_ffa_part_get(&res, host_ctxt); > > + break; > > } > > > > if (ffa_call_supported(func_id)) > > @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages) > > if (FFA_MAJOR_VERSION(res.a0) != 1) > > return -EOPNOTSUPP; > > > > + ffa_version = res.a0; > > + > > arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res); > > if (res.a0 != FFA_SUCCESS) > > return -EOPNOTSUPP; > > -- > > 2.44.0.478.gd926399ef9-goog > >
On Wed, Apr 10, 2024 at 06:10:37PM +0100, Vincent Donnefort wrote: Hi Vincent, > On Wed, Apr 10, 2024 at 10:18:18AM +0000, Sebastian Ene wrote: > > On Wed, Apr 10, 2024 at 10:53:31AM +0100, Vincent Donnefort wrote: > > > [...] > > > > > > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > > > > > + struct kvm_cpu_context *ctxt) > > > > > > +{ > > > > > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > > > > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > > > > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > > > > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > > > > > + DECLARE_REG(u32, flags, ctxt, 5); > > > > > > + u32 off, count, sz, buf_sz; > > > > > > + > > > > > > + hyp_spin_lock(&host_buffers.lock); > > > > > > + if (!host_buffers.rx) { > > > > > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > > > > > + goto out_unlock; > > > > > > + } > > > > > > + > > > > > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > > > > > + uuid2, uuid3, flags, 0, 0, > > > > > > + res); > > > > > > + > > > > > > + if (res->a0 != FFA_SUCCESS) > > > > > > + goto out_unlock; > > > > > > + > > > > > > + count = res->a2; > > > > > > + if (!count) > > > > > > + goto out_unlock; > > > > > > > > > > Looking at the table 13.34, it seems what's in "count" depends on the flag. > > > > > Shouldn't we check its value, and only memcpy into the host buffers if the flag > > > > > is 0? > > > > > > > > > > > > > When the flag is `1` the count referes to the number of partitions > > > > deployed. In both cases we have to copy something unless count == 0. > > > > > > I see "Return the count of partitions deployed in the system corresponding to > > > the specified UUID in w2" > > > > > > Which I believe means nothing has been copied in the buffer? > > > > > > > When the flag in w5 is 1 the size argument stored in w3 will be zero and > > the loop will not be executed, so nothing will be copied to the host > > buffers. > > Ha right, all good here then. > > > > > > > > > > > > > + > > > > > > + if (ffa_version > FFA_VERSION_1_0) { > > > > > > + buf_sz = sz = res->a3; > > > > > > + if (sz > sizeof(struct ffa_partition_info)) > > > > > > + buf_sz = sizeof(struct ffa_partition_info); > > > > > > > > > > What are you trying to protect against here? We have to trust EL3 anyway, (as > > > > > other functions do). > > > > > > > > > > The WARN() could be kept though to make sure we won't overflow our buffer. But > > > > > it could be transformed into an error? FFA_RET_ABORTED? > > > > > > > > > > > > > > > > > > I think we can keep it as a WARN_ON because it is not expected to have > > > > a return code of FFA_SUCCESS but the buffer to be overflown. The TEE is > > > > expected to return NO_MEMORY in w2 if the results cannot fit in the RX > > > > buffer. > > > > > > WARN() is crashing the hypervisor. It'd be a shame here as we can easily recover > > > by just sending an error back to the caller. > > > > I agree with you but this is not expected to happen unless TZ messes up > > something/is not complaint with the spec, in which case I would like to > > catch this. > > Hum, still I don't see the point in crashing anything here, nothing is > compromised. The driver can then decide what to do based on that reported > failure. I still think we should keep this (as we discussed offilne). We do WARN_ON when the behaviour doesn't follow the spec guidelines. For this particular case, if the result doesn't fit in the caller's buffer the TEE is expected to return FFA_ERROR in w0 with NO_MEMORY in w2. If it returns success but the caller doesn't have enough space to copy then something is going horribly wrong on the TEE side. Thanks, Seb
On Wed, Apr 10, 2024 at 04:57:19PM +0200, Lorenzo Pieralisi wrote: > On Tue, Apr 09, 2024 at 03:19:08PM +0000, Sebastian Ene wrote: > > Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor > > and copy the response message back to the host buffers. Save the > > returned FF-A version as we will need it later to interpret the response > > from the TEE. > > > > Signed-off-by: Sebastian Ene <sebastianene@google.com> > > --- > > arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c > > index 320f2eaa14a9..72fc365bc7a8 100644 > > --- a/arch/arm64/kvm/hyp/nvhe/ffa.c > > +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c > > @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { > > */ > > static struct kvm_ffa_buffers hyp_buffers; > > static struct kvm_ffa_buffers host_buffers; > > +static u32 ffa_version; > > > > static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) > > { > > @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, > > return true; > > } > > > > +static void do_ffa_part_get(struct arm_smccc_res *res, > > + struct kvm_cpu_context *ctxt) > > +{ > > + DECLARE_REG(u32, uuid0, ctxt, 1); > > + DECLARE_REG(u32, uuid1, ctxt, 2); > > + DECLARE_REG(u32, uuid2, ctxt, 3); > > + DECLARE_REG(u32, uuid3, ctxt, 4); > > + DECLARE_REG(u32, flags, ctxt, 5); > > + u32 off, count, sz, buf_sz; > > + > > + hyp_spin_lock(&host_buffers.lock); > > + if (!host_buffers.rx) { > > + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); > > + goto out_unlock; > > + } > > + > > + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, > > + uuid2, uuid3, flags, 0, 0, > > + res); > > + > > + if (res->a0 != FFA_SUCCESS) > > + goto out_unlock; > > + > > + count = res->a2; > > + if (!count) > > + goto out_unlock; > > + > > + if (ffa_version > FFA_VERSION_1_0) { > > + buf_sz = sz = res->a3; > > + if (sz > sizeof(struct ffa_partition_info)) > > + buf_sz = sizeof(struct ffa_partition_info); > > We are copying buf_sz but (correctly ?) returning res->a3 to the caller, > which is allowed to expect res->a3 bytes to be filled since that's what > firmware reported. > > Technically this is not a problem at present, because the caller > (ie the FF-A driver) and the hypervisor rely on the same descriptor > structures (and buf_sz can't be != sizeof(struct ffa_partition_info), > anything else is a bug as we stand); they must be kept in sync though as > the firmware version changes (*if* there are changes in the partition > descriptor - eg fields are added). > Indeed, this will break if the size of the descriptor changes in the future and the kernel has not yet added the support for that though it is unlikely as we negotiate the version and the response for all the messages should be as per the negotiated version. > An option would consist in just copying res->a3 bytes as firmware reports > (obviously keeping the RX buffer boundary checks for the memcpy). > Yes I prefer that. > It is just a heads-up because I noticed it, no more, I will let Sudeep > comment on this since he knows better. > I think you had it all covered, nothing much to add. -- Regards, Sudeep
diff --git a/arch/arm64/kvm/hyp/nvhe/ffa.c b/arch/arm64/kvm/hyp/nvhe/ffa.c index 320f2eaa14a9..72fc365bc7a8 100644 --- a/arch/arm64/kvm/hyp/nvhe/ffa.c +++ b/arch/arm64/kvm/hyp/nvhe/ffa.c @@ -67,6 +67,7 @@ struct kvm_ffa_buffers { */ static struct kvm_ffa_buffers hyp_buffers; static struct kvm_ffa_buffers host_buffers; +static u32 ffa_version; static void ffa_to_smccc_error(struct arm_smccc_res *res, u64 ffa_errno) { @@ -640,6 +641,49 @@ static bool do_ffa_features(struct arm_smccc_res *res, return true; } +static void do_ffa_part_get(struct arm_smccc_res *res, + struct kvm_cpu_context *ctxt) +{ + DECLARE_REG(u32, uuid0, ctxt, 1); + DECLARE_REG(u32, uuid1, ctxt, 2); + DECLARE_REG(u32, uuid2, ctxt, 3); + DECLARE_REG(u32, uuid3, ctxt, 4); + DECLARE_REG(u32, flags, ctxt, 5); + u32 off, count, sz, buf_sz; + + hyp_spin_lock(&host_buffers.lock); + if (!host_buffers.rx) { + ffa_to_smccc_res(res, FFA_RET_INVALID_PARAMETERS); + goto out_unlock; + } + + arm_smccc_1_1_smc(FFA_PARTITION_INFO_GET, uuid0, uuid1, + uuid2, uuid3, flags, 0, 0, + res); + + if (res->a0 != FFA_SUCCESS) + goto out_unlock; + + count = res->a2; + if (!count) + goto out_unlock; + + if (ffa_version > FFA_VERSION_1_0) { + buf_sz = sz = res->a3; + if (sz > sizeof(struct ffa_partition_info)) + buf_sz = sizeof(struct ffa_partition_info); + } else { + /* FFA_VERSION_1_0 lacks the size in the response */ + buf_sz = sz = 8; + } + + WARN_ON((count - 1) * sz + buf_sz > PAGE_SIZE); + for (off = 0; off < count * sz; off += sz) + memcpy(host_buffers.rx + off, hyp_buffers.rx + off, buf_sz); +out_unlock: + hyp_spin_unlock(&host_buffers.lock); +} + bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) { struct arm_smccc_res res; @@ -686,6 +730,9 @@ bool kvm_host_ffa_handler(struct kvm_cpu_context *host_ctxt, u32 func_id) case FFA_MEM_FRAG_TX: do_ffa_mem_frag_tx(&res, host_ctxt); goto out_handled; + case FFA_PARTITION_INFO_GET: + do_ffa_part_get(&res, host_ctxt); + break; } if (ffa_call_supported(func_id)) @@ -726,6 +773,8 @@ int hyp_ffa_init(void *pages) if (FFA_MAJOR_VERSION(res.a0) != 1) return -EOPNOTSUPP; + ffa_version = res.a0; + arm_smccc_1_1_smc(FFA_ID_GET, 0, 0, 0, 0, 0, 0, 0, &res); if (res.a0 != FFA_SUCCESS) return -EOPNOTSUPP;
Handle the FFA_PARTITION_INFO_GET host call inside the pKVM hypervisor and copy the response message back to the host buffers. Save the returned FF-A version as we will need it later to interpret the response from the TEE. Signed-off-by: Sebastian Ene <sebastianene@google.com> --- arch/arm64/kvm/hyp/nvhe/ffa.c | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)