hash_index is evaluated by looping phdrs till QCOM_MDT_TYPE_HASH is found. Add an upperbound check to phdrs to access within elf size.
Fixes: 64fb5eb87d58 ("soc: qcom: mdt_loader: Allow hash to reside in any segment") Cc: stable@vger.kernel.org Signed-off-by: Auditya Bhattaram quic_audityab@quicinc.com Acked-by: Mukesh Ojha quic_mojha@quicinc.com --- Changes in v4: - Added additional prints incase of Invalid access. Link to v3 https://lore.kernel.org/stable/1c91c653-cebe-4407-bdd6-cfc73b64c0fb@quicinc.... Link to v2 https://lore.kernel.org/linux-arm-msm/9773d189-c896-d5c5-804c-e086c24987b4@q... Link to v1 https://lore.kernel.org/linux-arm-msm/5d7a3b97-d840-4863-91a0-32c1d8e7532f@l... --- drivers/soc/qcom/mdt_loader.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 6f177e46fa0f..1a79a7bba468 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -145,6 +143,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, if (phdrs[0].p_type == PT_LOAD) return ERR_PTR(-EINVAL);
+ if (((size_t)(phdrs + ehdr->e_phnum)) > ((size_t)ehdr + fw->size)) { + dev_err(dev, + "Invalid phdrs access for fw: %s, e_phnum: %u, fw->size: %zu\n", + fw_name, ehdr->e_phnum, fw->size); + return ERR_PTR(-EINVAL); + } + for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) { hash_segment = i; -- 2.17.1
On Tue, Feb 13, 2024 at 01:30:10PM +0530, Auditya Bhattaram wrote:
hash_index is evaluated by looping phdrs till QCOM_MDT_TYPE_HASH is found. Add an upperbound check to phdrs to access within elf size.
How is this compatible with what is being observed on SM8450 and implemented in commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"?
Regards, Bjorn
Fixes: 64fb5eb87d58 ("soc: qcom: mdt_loader: Allow hash to reside in any segment") Cc: stable@vger.kernel.org Signed-off-by: Auditya Bhattaram quic_audityab@quicinc.com Acked-by: Mukesh Ojha quic_mojha@quicinc.com
Changes in v4:
- Added additional prints incase of Invalid access.
Link to v3 https://lore.kernel.org/stable/1c91c653-cebe-4407-bdd6-cfc73b64c0fb@quicinc.... Link to v2 https://lore.kernel.org/linux-arm-msm/9773d189-c896-d5c5-804c-e086c24987b4@q... Link to v1 https://lore.kernel.org/linux-arm-msm/5d7a3b97-d840-4863-91a0-32c1d8e7532f@l...
drivers/soc/qcom/mdt_loader.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 6f177e46fa0f..1a79a7bba468 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -145,6 +143,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, if (phdrs[0].p_type == PT_LOAD) return ERR_PTR(-EINVAL);
- if (((size_t)(phdrs + ehdr->e_phnum)) > ((size_t)ehdr + fw->size)) {
dev_err(dev,"Invalid phdrs access for fw: %s, e_phnum: %u, fw->size: %zu\n",fw_name, ehdr->e_phnum, fw->size);return ERR_PTR(-EINVAL);- }
- for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) { hash_segment = i;
-- 2.17.1
On 2/14/2024 11:12 AM, Bjorn Andersson wrote:
On Tue, Feb 13, 2024 at 01:30:10PM +0530, Auditya Bhattaram wrote:
hash_index is evaluated by looping phdrs till QCOM_MDT_TYPE_HASH is found. Add an upperbound check to phdrs to access within elf size.
How is this compatible with what is being observed on SM8450 and implemented in commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"?
Regards, Bjorn
Calculating hash_index is introduced with this commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"
for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) ...
I'm trying to add an upper bound for this access "phdrs[i]"
Fixes: 64fb5eb87d58 ("soc: qcom: mdt_loader: Allow hash to reside in any segment") Cc: stable@vger.kernel.org Signed-off-by: Auditya Bhattaram quic_audityab@quicinc.com Acked-by: Mukesh Ojha quic_mojha@quicinc.com
Changes in v4:
- Added additional prints incase of Invalid access.
Link to v3 https://lore.kernel.org/stable/1c91c653-cebe-4407-bdd6-cfc73b64c0fb@quicinc.... Link to v2 https://lore.kernel.org/linux-arm-msm/9773d189-c896-d5c5-804c-e086c24987b4@q... Link to v1 https://lore.kernel.org/linux-arm-msm/5d7a3b97-d840-4863-91a0-32c1d8e7532f@l...
drivers/soc/qcom/mdt_loader.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 6f177e46fa0f..1a79a7bba468 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -145,6 +143,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, if (phdrs[0].p_type == PT_LOAD) return ERR_PTR(-EINVAL);
- if (((size_t)(phdrs + ehdr->e_phnum)) > ((size_t)ehdr + fw->size)) {
dev_err(dev,"Invalid phdrs access for fw: %s, e_phnum: %u, fw->size: %zu\n",fw_name, ehdr->e_phnum, fw->size);return ERR_PTR(-EINVAL);- }
- for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) { hash_segment = i;
-- 2.17.1
On 2/14/2024 11:27 AM, Auditya Bhattaram wrote:
On 2/14/2024 11:12 AM, Bjorn Andersson wrote:
On Tue, Feb 13, 2024 at 01:30:10PM +0530, Auditya Bhattaram wrote:
hash_index is evaluated by looping phdrs till QCOM_MDT_TYPE_HASH is found. Add an upperbound check to phdrs to access within elf size.
How is this compatible with what is being observed on SM8450 and implemented in commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"?
Regards, Bjorn
Calculating hash_index is introduced with this commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"
for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) ...
I'm trying to add an upper bound for this access "phdrs[i]"
Any further questions on this Bjorn.
Fixes: 64fb5eb87d58 ("soc: qcom: mdt_loader: Allow hash to reside in any segment") Cc: stable@vger.kernel.org Signed-off-by: Auditya Bhattaram quic_audityab@quicinc.com Acked-by: Mukesh Ojha quic_mojha@quicinc.com
Changes in v4: - Added additional prints incase of Invalid access. Link to v3 https://lore.kernel.org/stable/1c91c653-cebe-4407-bdd6-cfc73b64c0fb@quicinc.... Link to v2 https://lore.kernel.org/linux-arm-msm/9773d189-c896-d5c5-804c-e086c24987b4@q... Link to v1 https://lore.kernel.org/linux-arm-msm/5d7a3b97-d840-4863-91a0-32c1d8e7532f@l...
drivers/soc/qcom/mdt_loader.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 6f177e46fa0f..1a79a7bba468 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -145,6 +143,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, if (phdrs[0].p_type == PT_LOAD) return ERR_PTR(-EINVAL);
+ if (((size_t)(phdrs + ehdr->e_phnum)) > ((size_t)ehdr + fw->size)) { + dev_err(dev, + "Invalid phdrs access for fw: %s, e_phnum: %u, fw->size: %zu\n", + fw_name, ehdr->e_phnum, fw->size); + return ERR_PTR(-EINVAL); + }
for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) { hash_segment = i; -- 2.17.1
On Wed, Feb 14, 2024 at 11:27:01AM +0530, Auditya Bhattaram wrote:
On 2/14/2024 11:12 AM, Bjorn Andersson wrote:
On Tue, Feb 13, 2024 at 01:30:10PM +0530, Auditya Bhattaram wrote:
hash_index is evaluated by looping phdrs till QCOM_MDT_TYPE_HASH is found. Add an upperbound check to phdrs to access within elf size.
How is this compatible with what is being observed on SM8450 and implemented in commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"?
Regards, Bjorn
Calculating hash_index is introduced with this commit 8bd42e2341a7 ("soc: qcom: mdt_loader: Allow hash segment to be split out"
for (i = 1; i < ehdr->e_phnum; i++) {if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) ...
I'm trying to add an upper bound for this access "phdrs[i]"
Ahh, sorry, you're of course correct. I think I would have preferred a more obvious check of offset >= fw->size though...
That said, this code used to sit behind rproc_elf_sanity_check(), but that seem to have been lost in the refactorings. As such, your patch is incomplete and we should reintroduce a whole bunch of these sanity checks!
Regards, Bjorn
Fixes: 64fb5eb87d58 ("soc: qcom: mdt_loader: Allow hash to reside in any segment") Cc: stable@vger.kernel.org Signed-off-by: Auditya Bhattaram quic_audityab@quicinc.com Acked-by: Mukesh Ojha quic_mojha@quicinc.com
Changes in v4:
- Added additional prints incase of Invalid access.
Link to v3 https://lore.kernel.org/stable/1c91c653-cebe-4407-bdd6-cfc73b64c0fb@quicinc.... Link to v2 https://lore.kernel.org/linux-arm-msm/9773d189-c896-d5c5-804c-e086c24987b4@q... Link to v1 https://lore.kernel.org/linux-arm-msm/5d7a3b97-d840-4863-91a0-32c1d8e7532f@l...
drivers/soc/qcom/mdt_loader.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c index 6f177e46fa0f..1a79a7bba468 100644 --- a/drivers/soc/qcom/mdt_loader.c +++ b/drivers/soc/qcom/mdt_loader.c @@ -145,6 +143,13 @@ void *qcom_mdt_read_metadata(const struct firmware *fw, size_t *data_len, if (phdrs[0].p_type == PT_LOAD) return ERR_PTR(-EINVAL);
- if (((size_t)(phdrs + ehdr->e_phnum)) > ((size_t)ehdr + fw->size)) {
dev_err(dev,"Invalid phdrs access for fw: %s, e_phnum: %u, fw->size: %zu\n",fw_name, ehdr->e_phnum, fw->size);return ERR_PTR(-EINVAL);- }
- for (i = 1; i < ehdr->e_phnum; i++) { if ((phdrs[i].p_flags & QCOM_MDT_TYPE_MASK) == QCOM_MDT_TYPE_HASH) { hash_segment = i;
-- 2.17.1
linux-stable-mirror@lists.linaro.org
-
Auditya Bhattaram -
Bjorn Andersson