A set of patches for bug fixes and improved logging in mhi/core/boot.c.
Verified on x86 and arm64 platforms.
v2:
-Fix channel ID range check potential infinite loop
-Add appropriate signed-off-by tags
Bhaumik Bhatt (5):
bus: mhi: core: Handle firmware load using state worker
bus: mhi: core: WARN_ON for malformed vector table
bus: mhi: core: Return appropriate error codes for AMSS load failure
bus: mhi: core: Improve debug logs for loading firmware
bus: mhi: core: Ensure non-zero session or sequence ID values
Hemant Kumar (3):
bus: mhi: core: Cache intmod from mhi event to mhi channel
bus: mhi: core: Add range check for channel id received in event ring
bus: mhi: core: Read transfer length from an event properly
drivers/bus/mhi/core/boot.c | 74 +++++++++++++++++++++++++----------------
drivers/bus/mhi/core/init.c | 5 ++-
drivers/bus/mhi/core/internal.h | 1 +
drivers/bus/mhi/core/main.c | 18 +++++++---
drivers/bus/mhi/core/pm.c | 6 +---
include/linux/mhi.h | 2 --
6 files changed, 65 insertions(+), 41 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
When loading AMSS firmware using BHIe protocol, return -ETIMEDOUT if no
response is received within the timeout or return -EIO in case of a
protocol returned failure or an MHI error state.
Signed-off-by: Bhaumik Bhatt <[email protected]>
Signed-off-by: Hemant Kumar <[email protected]>
---
drivers/bus/mhi/core/boot.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index bc70edc..4e49a0e 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -176,6 +176,7 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
void __iomem *base = mhi_cntrl->bhie;
rwlock_t *pm_lock = &mhi_cntrl->pm_lock;
u32 tx_status, sequence_id;
+ int ret;
read_lock_bh(pm_lock);
if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state)) {
@@ -198,19 +199,19 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
read_unlock_bh(pm_lock);
/* Wait for the image download to complete */
- wait_event_timeout(mhi_cntrl->state_event,
- MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
- mhi_read_reg_field(mhi_cntrl, base,
- BHIE_TXVECSTATUS_OFFS,
- BHIE_TXVECSTATUS_STATUS_BMSK,
- BHIE_TXVECSTATUS_STATUS_SHFT,
- &tx_status) || tx_status,
- msecs_to_jiffies(mhi_cntrl->timeout_ms));
-
- if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state))
+ ret = wait_event_timeout(mhi_cntrl->state_event,
+ MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
+ mhi_read_reg_field(mhi_cntrl, base,
+ BHIE_TXVECSTATUS_OFFS,
+ BHIE_TXVECSTATUS_STATUS_BMSK,
+ BHIE_TXVECSTATUS_STATUS_SHFT,
+ &tx_status) || tx_status,
+ msecs_to_jiffies(mhi_cntrl->timeout_ms));
+ if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state) ||
+ tx_status != BHIE_TXVECSTATUS_STATUS_XFER_COMPL)
return -EIO;
- return (tx_status == BHIE_TXVECSTATUS_STATUS_XFER_COMPL) ? 0 : -EIO;
+ return (!ret) ? -ETIMEDOUT : 0;
}
static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Hemant Kumar <[email protected]>
MHI data completion handler function reads channel id from event
ring element. Value is under the control of MHI devices and can be
any value between 0 and 255. In order to prevent out of bound access
add a bound check against the max channel supported by controller
and skip processing of that event ring element.
Signed-off-by: Hemant Kumar <[email protected]>
Signed-off-by: Bhaumik Bhatt <[email protected]>
---
drivers/bus/mhi/core/main.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 23154f1..1ccd4cc 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
chan = MHI_TRE_GET_EV_CHID(local_rp);
+ if (WARN_ON(chan >= mhi_cntrl->max_chan))
+ goto next_event;
+
mhi_chan = &mhi_cntrl->mhi_chan[chan];
if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
@@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
event_quota--;
}
+next_event:
mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
local_rp = ev_ring->rp;
dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Hemant Kumar <[email protected]>
When MHI Driver receives an EOT event, it reads xfer_len from the
event in the last TRE. The value is under control of the MHI device
and never validated by Host MHI driver. The value should never be
larger than the real size of the buffer but a malicious device can
set the value 0xFFFF as maximum. This causes device to memory
overflow (both read or write). Fix this issue by reading minimum of
transfer length from event and the buffer length provided.
Signed-off-by: Hemant Kumar <[email protected]>
Signed-off-by: Bhaumik Bhatt <[email protected]>
---
drivers/bus/mhi/core/main.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 1ccd4cc..3d468d9 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
result.buf_addr = buf_info->cb_buf;
- result.bytes_xferd = xfer_len;
+
+ /* truncate to buf len if xfer_len is larger */
+ result.bytes_xferd =
+ min_t(u16, xfer_len, buf_info->len);
mhi_del_ring_element(mhi_cntrl, buf_ring);
mhi_del_ring_element(mhi_cntrl, tre_ring);
local_rp = tre_ring->rp;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <[email protected]>
>
> MHI data completion handler function reads channel id from event
> ring element. Value is under the control of MHI devices and can be
> any value between 0 and 255. In order to prevent out of bound access
> add a bound check against the max channel supported by controller
> and skip processing of that event ring element.
>
> Signed-off-by: Hemant Kumar <[email protected]>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
> ---
> drivers/bus/mhi/core/main.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 23154f1..1ccd4cc 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
>
> chan = MHI_TRE_GET_EV_CHID(local_rp);
> + if (WARN_ON(chan >= mhi_cntrl->max_chan))
> + goto next_event;
> +
> mhi_chan = &mhi_cntrl->mhi_chan[chan];
>
> if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
> @@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
> event_quota--;
> }
>
> +next_event:
> mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
> local_rp = ev_ring->rp;
> dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
>
It looks like the same issue exists in mhi_process_ctrl_ev_ring(), and
thus some form of this solution needs to be applied there as well.
Would you please fix that too?
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
> From: Hemant Kumar <[email protected]>
>
> When MHI Driver receives an EOT event, it reads xfer_len from the
> event in the last TRE. The value is under control of the MHI device
> and never validated by Host MHI driver. The value should never be
> larger than the real size of the buffer but a malicious device can
> set the value 0xFFFF as maximum. This causes device to memory
The device will overflow, or the driver?
> overflow (both read or write). Fix this issue by reading minimum of
> transfer length from event and the buffer length provided.
>
> Signed-off-by: Hemant Kumar <[email protected]>
> Signed-off-by: Bhaumik Bhatt <[email protected]>
> ---
> drivers/bus/mhi/core/main.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1ccd4cc..3d468d9 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
> mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
>
> result.buf_addr = buf_info->cb_buf;
> - result.bytes_xferd = xfer_len;
> +
> + /* truncate to buf len if xfer_len is larger */
> + result.bytes_xferd =
> + min_t(u16, xfer_len, buf_info->len);
> mhi_del_ring_element(mhi_cntrl, buf_ring);
> mhi_del_ring_element(mhi_cntrl, tre_ring);
> local_rp = tre_ring->rp;
>
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Jeff
On 4/28/20 7:44 AM, Jeffrey Hugo wrote:
> On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <[email protected]>
>>
>> MHI data completion handler function reads channel id from event
>> ring element. Value is under the control of MHI devices and can be
>> any value between 0 and 255. In order to prevent out of bound access
>> add a bound check against the max channel supported by controller
>> and skip processing of that event ring element.
>>
>> Signed-off-by: Hemant Kumar <[email protected]>
>> Signed-off-by: Bhaumik Bhatt <[email protected]>
>> ---
>> drivers/bus/mhi/core/main.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 23154f1..1ccd4cc 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -827,6 +827,9 @@ int mhi_process_data_event_ring(struct
>> mhi_controller *mhi_cntrl,
>> enum mhi_pkt_type type = MHI_TRE_GET_EV_TYPE(local_rp);
>> chan = MHI_TRE_GET_EV_CHID(local_rp);
>> + if (WARN_ON(chan >= mhi_cntrl->max_chan))
>> + goto next_event;
>> +
>> mhi_chan = &mhi_cntrl->mhi_chan[chan];
>> if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
>> @@ -837,6 +840,7 @@ int mhi_process_data_event_ring(struct
>> mhi_controller *mhi_cntrl,
>> event_quota--;
>> }
>> +next_event:
>> mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
>> local_rp = ev_ring->rp;
>> dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp);
>>
>
> It looks like the same issue exists in mhi_process_ctrl_ev_ring(), and
> thus some form of this solution needs to be applied there as well. Would
> you please fix that too?
>
As discussed with you off line, spec allows to have just event ring to
be used for both data and control. Updating this in V3.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Jeff
On 4/28/20 7:50 AM, Jeffrey Hugo wrote:
> On 4/27/2020 8:59 PM, Bhaumik Bhatt wrote:
>> From: Hemant Kumar <[email protected]>
>>
>> When MHI Driver receives an EOT event, it reads xfer_len from the
>> event in the last TRE. The value is under control of the MHI device
>> and never validated by Host MHI driver. The value should never be
>> larger than the real size of the buffer but a malicious device can
>> set the value 0xFFFF as maximum. This causes device to memory
>
> The device will overflow, or the driver?
Done.
>
>> overflow (both read or write). Fix this issue by reading minimum of
>> transfer length from event and the buffer length provided.
>>
>> Signed-off-by: Hemant Kumar <[email protected]>
>> Signed-off-by: Bhaumik Bhatt <[email protected]>
>> ---
>> drivers/bus/mhi/core/main.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 1ccd4cc..3d468d9 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -521,7 +521,10 @@ static int parse_xfer_event(struct mhi_controller
>> *mhi_cntrl,
>> mhi_cntrl->unmap_single(mhi_cntrl, buf_info);
>> result.buf_addr = buf_info->cb_buf;
>> - result.bytes_xferd = xfer_len;
>> +
>> + /* truncate to buf len if xfer_len is larger */
>> + result.bytes_xferd =
>> + min_t(u16, xfer_len, buf_info->len);
>> mhi_del_ring_element(mhi_cntrl, buf_ring);
>> mhi_del_ring_element(mhi_cntrl, tre_ring);
>> local_rp = tre_ring->rp;
>>
>
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project