| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2c0b4220-d92c-4dba-9364-33d713f26dbe@quicinc.com>
Date: Wed, 21 May 2025 09:32:04 -0600
From: Jeffrey Hugo <quic_jhugo@...cinc.com>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
"Bjorn
Helgaas" <bhelgaas@...gle.com>,
Ilpo Järvinen
<ilpo.jarvinen@...ux.intel.com>,
Jingoo Han <jingoohan1@...il.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Lorenzo Pieralisi
<lpieralisi@...nel.org>,
Krzysztof Wilczyński
<kw@...ux.com>,
Rob Herring <robh@...nel.org>,
Johannes Berg
<johannes@...solutions.net>,
Jeff Johnson <jjohnson@...nel.org>,
"Bartosz
Golaszewski" <brgl@...ev.pl>
CC: <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <mhi@...ts.linux.dev>,
<linux-wireless@...r.kernel.org>, <ath11k@...ts.infradead.org>,
<qiang.yu@....qualcomm.com>, <quic_vbadigan@...cinc.com>,
<quic_vpernami@...cinc.com>, <quic_mrana@...cinc.com>,
Jeff Johnson
<jeff.johnson@....qualcomm.com>
Subject: Re: [PATCH v3 07/11] bus: mhi: host: Add support to read MHI
capabilities
On 5/21/2025 9:06 AM, Krishna Chaitanya Chundru wrote:
>
>
> On 5/21/2025 8:22 PM, Jeffrey Hugo wrote:
>> On 5/19/2025 3:42 AM, Krishna Chaitanya Chundru wrote:
>>> From: Vivek Pernamitta <quic_vpernami@...cinc.com>
>>>
>>> As per MHI spec v1.2,sec 6.6, MHI has capability registers which are
>>> located after the ERDB array. The location of this group of registers is
>>> indicated by the MISCOFF register. Each capability has a capability
>>> ID to
>>> determine which functionality is supported and each capability will
>>> point
>>> to the next capability supported.
>>>
>>> Add a basic function to read those capabilities offsets.
>>>
>>> Signed-off-by: Vivek Pernamitta <quic_vpernami@...cinc.com>
>>> Signed-off-by: Krishna Chaitanya Chundru
>>> <krishna.chundru@....qualcomm.com>
>>> ---
>>> drivers/bus/mhi/common.h | 4 ++++
>>> drivers/bus/mhi/host/init.c | 29 +++++++++++++++++++++++++++++
>>> 2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
>>> index
>>> dda340aaed95a5573a2ec776ca712e11a1ed0b52..eedac801b80021e44f7c65d33cd50760e06c02f2 100644
>>> --- a/drivers/bus/mhi/common.h
>>> +++ b/drivers/bus/mhi/common.h
>>> @@ -16,6 +16,7 @@
>>> #define MHICFG 0x10
>>> #define CHDBOFF 0x18
>>> #define ERDBOFF 0x20
>>> +#define MISCOFF 0x24
>>> #define BHIOFF 0x28
>>> #define BHIEOFF 0x2c
>>> #define DEBUGOFF 0x30
>>> @@ -113,6 +114,9 @@
>>> #define MHISTATUS_MHISTATE_MASK GENMASK(15, 8)
>>> #define MHISTATUS_SYSERR_MASK BIT(2)
>>> #define MHISTATUS_READY_MASK BIT(0)
>>> +#define MISC_CAP_MASK GENMASK(31, 0)
>>> +#define CAP_CAPID_MASK GENMASK(31, 24)
>>> +#define CAP_NEXT_CAP_MASK GENMASK(23, 12)
>>> /* Command Ring Element macros */
>>> /* No operation command */
>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>> index
>>> 13e7a55f54ff45b83b3f18b97e2cdd83d4836fe3..a7137a040bdce1c58c98fe9c2340aae4cc4387d1 100644
>>> --- a/drivers/bus/mhi/host/init.c
>>> +++ b/drivers/bus/mhi/host/init.c
>>> @@ -467,6 +467,35 @@ int mhi_init_dev_ctxt(struct mhi_controller
>>> *mhi_cntrl)
>>> return ret;
>>> }
>>> +static int mhi_find_capability(struct mhi_controller *mhi_cntrl, u32
>>> capability, u32 *offset)
>>> +{
>>> + u32 val, cur_cap, next_offset;
>>> + int ret;
>>> +
>>> + /* Get the 1st supported capability offset */
>>
>> "first"? Does not seem like you are short on space here.
>>
> Misc register will have the offest of the 1st capability register
> from there capabilities will have linked list format.
No, I'm saying don't have "1st" in the comment, but have "first"
instead. I think that will read better.
"Get the first supported capability offset"
>>> + ret = mhi_read_reg_field(mhi_cntrl, mhi_cntrl->regs, MISCOFF,
>>> + MISC_CAP_MASK, offset);
>>
>> This fits on one line.
>>
>>> + if (ret)
>>> + return ret;
>>
>> Blank line here would be nice.
>>
>>> + do {
>>> + if (*offset >= mhi_cntrl->reg_len)
>>> + return -ENXIO;
>>> +
>>> + ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->regs, *offset, &val);
>>> + if (ret)
>>> + return ret;
>>
>>
>> There is no sanity checking we can do on val? We've had plenty of
>> issues blindly trusting the device. I would like to avoid having more.
>>
> we can check if val is not all F's as sanity other than that we can't
> check any other things as we don't know if the value is valid or not.
> Let me know if you have any taught on this.
mhi_read_reg() should handle the PCIe link error case. You can check to
see that the value is within the MHI space before using it.
Also, shouldn't the value be transformed from le32 to cpu and back?
>> Also looks like if we find the capability we are looking for, we
>> return the offset without validating it.
>>
> For offset I can have a check to make sure the offset is not crossing
> mhi reg len like above.
>
> - Krishna Chaitanya.
>>> +
>>> + cur_cap = FIELD_GET(CAP_CAPID_MASK, val);
>>> + next_offset = FIELD_GET(CAP_NEXT_CAP_MASK, val);
>>> + if (cur_cap == capability)
>>> + return 0;
>>> +
>>> + *offset = next_offset;
>>> + } while (next_offset);
>>> +
>>> + return -ENXIO;
>>> +}
>>> +
>>> int mhi_init_mmio(struct mhi_controller *mhi_cntrl)
>>> {
>>> u32 val;
>>>
>>
Powered by blists - more mailing lists