| Message ID | 20240227145500.299683-2-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/2] platform/x86/amd/pmf: Do not use readl() for policy buffer access | expand |
Hi Shyam & Armin, Shyam, please take a look at the question below. On Tue, 27 Feb 2024, Armin Wolf wrote: > The length of the policy buffer is not validated before accessing it, > which means that multiple out-of-bounds memory accesses can occur. > > This is especially bad since userspace can load policy binaries over > debugfs. > + if (dev->policy_sz < POLICY_COOKIE_LEN + sizeof(length)) > + return -EINVAL; > + > cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET); > length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN); This starts to feel like adding a struct for the header(?) would be better course of action here as then one could compare against sizeof(*header) and avoid all those casts (IMO, just access the header fields directly w/o the local variables). Shyam, do you think a struct makes sense here? There's some header in this policy, right? There are more thing to address here... 1) amd_pmf_start_policy_engine() function returns -EINVAL & res that is TA_PMF_* which inconsistent in type of the return value 2) Once 1) is fixed, the caller shadowing the return code can be fixed as well: ret = amd_pmf_start_policy_engine(dev); if (ret) return -EINVAL;
Hi Ilpo, On 2/27/2024 21:15, Ilpo Järvinen wrote: > Hi Shyam & Armin, > > Shyam, please take a look at the question below. > > On Tue, 27 Feb 2024, Armin Wolf wrote: > >> The length of the policy buffer is not validated before accessing it, >> which means that multiple out-of-bounds memory accesses can occur. >> >> This is especially bad since userspace can load policy binaries over >> debugfs. > IMO, this patch is not required, reason being: - the debugfs patch gets created only when CONFIG_AMD_PMF_DEBUG is enabled. - Sideload of policy binaries is only supported with a valid signing key. (think like this can be tested & verified within AMD environment) - Also, in amd_pmf_get_pb_data() there are boundary conditions that are being checked. Is that not sufficient enough? >> + if (dev->policy_sz < POLICY_COOKIE_LEN + sizeof(length)) >> + return -EINVAL; >> + >> cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET); >> length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN); > > This starts to feel like adding a struct for the header(?) would be better > course of action here as then one could compare against sizeof(*header) > and avoid all those casts (IMO, just access the header fields directly > w/o the local variables). Not sure if I get your question clearly. Can you elaborate a bit more on the struct you are envisioning? but IHMO, we actually don't need a struct - as all that we would need is to make sure the signing cookie is part of the policy binary and leave the rest of the error handling to ASP/TEE modules (we can rely on the feedback from those modules). > > Shyam, do you think a struct makes sense here? There's some header in > this policy, right? Yes, the policy binary on a whole has multiple sections within it and there are multiple headers (like signing, OEM header, etc). But that might be not real interest to the PMF driver. The only thing the driver has to make sure is that the policy binary going into ASP (AMD Secure Processor) is with in the limits and has a valid signing cookie. So this part is already taken care in the current code. > > > There are more thing to address here... > > 1) amd_pmf_start_policy_engine() function returns -EINVAL & res that is > TA_PMF_* which inconsistent in type of the return value > ah! it has mix of both int and u32 :-) Armin, would you like to amend this in your current series? or else I will submit a change for this in my next series. Thanks, Shyam > 2) Once 1) is fixed, the caller shadowing the return code can be fixed as > well: > ret = amd_pmf_start_policy_engine(dev); > if (ret) > return -EINVAL; > >
Am 28.02.24 um 12:16 schrieb Shyam Sundar S K: > Hi Ilpo, > > On 2/27/2024 21:15, Ilpo Järvinen wrote: >> Hi Shyam & Armin, >> >> Shyam, please take a look at the question below. >> >> On Tue, 27 Feb 2024, Armin Wolf wrote: >> >>> The length of the policy buffer is not validated before accessing it, >>> which means that multiple out-of-bounds memory accesses can occur. >>> >>> This is especially bad since userspace can load policy binaries over >>> debugfs. > IMO, this patch is not required, reason being: > - the debugfs patch gets created only when CONFIG_AMD_PMF_DEBUG is > enabled. > - Sideload of policy binaries is only supported with a valid signing > key. (think like this can be tested & verified within AMD environment) > - Also, in amd_pmf_get_pb_data() there are boundary conditions that > are being checked. Is that not sufficient enough? IMHO, amd_pmf_get_pb_data() only checks if the length of the binary is between 0 and the maximum buffer size. If for example the binary contains only 4 bytes, then there will be an out-of-bounds access when trying to read the cookie and length. Or if the length is bigger than the binary buffer, then the driver just updates the buffer length even if the buffer is too small. I think the driver should catch such cases and return an error. (Please note that we are talking about the binary buffer, not the internal structure of the remaining policy binary itself). >>> + if (dev->policy_sz < POLICY_COOKIE_LEN + sizeof(length)) >>> + return -EINVAL; >>> + >>> cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET); >>> length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN); >> This starts to feel like adding a struct for the header(?) would be better >> course of action here as then one could compare against sizeof(*header) >> and avoid all those casts (IMO, just access the header fields directly >> w/o the local variables). > Not sure if I get your question clearly. Can you elaborate a bit more > on the struct you are envisioning? I think he envisions something like this: struct __packed cookie_header { u32 magic; u32 length; }; > > but IHMO, we actually don't need a struct - as all that we would need > is to make sure the signing cookie is part of the policy binary and > leave the rest of the error handling to ASP/TEE modules (we can rely > on the feedback from those modules). > >> Shyam, do you think a struct makes sense here? There's some header in >> this policy, right? > Yes, the policy binary on a whole has multiple sections within it and > there are multiple headers (like signing, OEM header, etc). > > But that might be not real interest to the PMF driver. The only thing > the driver has to make sure is that the policy binary going into ASP > (AMD Secure Processor) is with in the limits and has a valid signing > cookie. So this part is already taken care in the current code. > >> >> There are more thing to address here... >> >> 1) amd_pmf_start_policy_engine() function returns -EINVAL & res that is >> TA_PMF_* which inconsistent in type of the return value >> > ah! it has mix of both int and u32 :-) > > Armin, would you like to amend this in your current series? or else I > will submit a change for this in my next series. > > Thanks, > Shyam I can do so, but i will be unable to send a new patch series for the rest of this week. Thanks, Armin Wolf >> 2) Once 1) is fixed, the caller shadowing the return code can be fixed as >> well: >> ret = amd_pmf_start_policy_engine(dev); >> if (ret) >> return -EINVAL; >> >>
On Wed, 28 Feb 2024, Armin Wolf wrote: > Am 28.02.24 um 12:16 schrieb Shyam Sundar S K: > > On 2/27/2024 21:15, Ilpo Järvinen wrote: > > > On Tue, 27 Feb 2024, Armin Wolf wrote: > > > > > > > The length of the policy buffer is not validated before accessing it, > > > > which means that multiple out-of-bounds memory accesses can occur. > > > > > > > > This is especially bad since userspace can load policy binaries over > > > > debugfs. > > IMO, this patch is not required, reason being: > > - the debugfs patch gets created only when CONFIG_AMD_PMF_DEBUG is > > enabled. > > - Sideload of policy binaries is only supported with a valid signing > > key. (think like this can be tested & verified within AMD environment) > > - Also, in amd_pmf_get_pb_data() there are boundary conditions that > > are being checked. Is that not sufficient enough? > > IMHO, amd_pmf_get_pb_data() only checks if the length of the binary is > between 0 and the maximum buffer size. > > If for example the binary contains only 4 bytes, then there will be an > out-of-bounds access when trying to read the cookie and length. > > Or if the length is bigger than the binary buffer, then the driver just > updates the buffer length even if the buffer is too small. > > I think the driver should catch such cases and return an error. > > (Please note that we are talking about the binary buffer, not the internal > structure of the remaining policy binary itself). Yes. Out of bound accesses are not okay during validation even if the binary itself would get rejected at a later stage. It doesn't matter if the interface is only for debug or wider scope. > > > > + if (dev->policy_sz < POLICY_COOKIE_LEN + sizeof(length)) > > > > + return -EINVAL; > > > > + > > > > cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET); > > > > length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN); > > > This starts to feel like adding a struct for the header(?) would be better > > > course of action here as then one could compare against sizeof(*header) > > > and avoid all those casts (IMO, just access the header fields directly > > > w/o the local variables). > > Not sure if I get your question clearly. Can you elaborate a bit more > > on the struct you are envisioning? > > I think he envisions something like this: > > struct __packed cookie_header { > u32 magic; > u32 length; > }; > > > > > but IHMO, we actually don't need a struct - as all that we would need > > is to make sure the signing cookie is part of the policy binary and > > leave the rest of the error handling to ASP/TEE modules (we can rely > > on the feedback from those modules). > > > > > Shyam, do you think a struct makes sense here? There's some header in > > > this policy, right? > > Yes, the policy binary on a whole has multiple sections within it and > > there are multiple headers (like signing, OEM header, etc). > > > > But that might be not real interest to the PMF driver. The only thing > > the driver has to make sure is that the policy binary going into ASP > > (AMD Secure Processor) is with in the limits and has a valid signing > > cookie. So this part is already taken care in the current code. Clearly the PMF driver is interested in the header which contains the cookie and length fields as proven by the code (even if that's for the validation purposes only). I'm not asking about add various other headers which are of no interest. > > > There are more thing to address here... > > > > > > 1) amd_pmf_start_policy_engine() function returns -EINVAL & res that is > > > TA_PMF_* which inconsistent in type of the return value > > > > > ah! it has mix of both int and u32 :-) I was talking more on a logical level not so much about C type itself. It is confusing to return error in two ways: as -Exx values and TA_PMF_* which are positive. As a general rule (IMO), the HW specific errors should be mapped to -Exx codes at latest when a function returns also -Exx code which is the case here.
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c index b3491268b6a0..09e3c620a9c7 100644 --- a/drivers/platform/x86/amd/pmf/tee-if.c +++ b/drivers/platform/x86/amd/pmf/tee-if.c @@ -249,12 +249,18 @@ static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev) u32 cookie, length; int res; + if (dev->policy_sz < POLICY_COOKIE_LEN + sizeof(length)) + return -EINVAL; + cookie = *(u32 *)(dev->policy_buf + POLICY_COOKIE_OFFSET); length = *(u32 *)(dev->policy_buf + POLICY_COOKIE_LEN); if (cookie != POLICY_SIGN_COOKIE || !length) return -EINVAL; + if (dev->policy_sz < length + 512) + return -EINVAL; + /* Update the actual length */ dev->policy_sz = length + 512; res = amd_pmf_invoke_cmd_init(dev);
The length of the policy buffer is not validated before accessing it, which means that multiple out-of-bounds memory accesses can occur. This is especially bad since userspace can load policy binaries over debugfs. Compile-tested only. Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- Changes since v1: - check if the policy buffer also has enough room for storing the length --- drivers/platform/x86/amd/pmf/tee-if.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.39.2