| 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: <bfd5c9c2-ba3d-4222-8586-9620aa2ec898@oss.qualcomm.com>
Date: Fri, 1 Aug 2025 16:18:19 -0700
From: Anjelique Melendez <anjelique.melendez@....qualcomm.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: heikki.krogerus@...ux.intel.com, lumag@...nel.org,
neil.armstrong@...aro.org, johan+linaro@...nel.org,
quic_bjorande@...cinc.com, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] usb: typec: ucsi: ucsi_glink: Increase buffer size to
support UCSI v2
On 7/15/2025 10:06 PM, Greg KH wrote:
> On Tue, Jul 15, 2025 at 05:52:24PM -0700, Anjelique Melendez wrote:
>> UCSI v2 specification has increased the MSG_IN and MSG_OUT size from
>> 16 bytes to 256 bytes each for the message exchange between OPM and PPM
>> This makes the total buffer size increase from 48 bytes to 528 bytes.
>> Update the buffer size to support this increase.
>>
>> Signed-off-by: Anjelique Melendez <anjelique.melendez@....qualcomm.com>
>> ---
>> Changes since v1:
>> - Defined buf size in terms of other UCSI defines
>> - Removed UCSI_BUF_SIZE and used the explicit v1 or v2 buffer size macros
>> - Removed Qualcomm copyright
>> - link: https://lore.kernel.org/all/20250624222922.2010820-1-anjelique.melendez@oss.qualcomm.com/
>> ---
>> drivers/usb/typec/ucsi/ucsi_glink.c | 58 ++++++++++++++++++++++++-----
>> 1 file changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
>> index 8af79101a2fc..2918c88e54d2 100644
>> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
>> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
>> @@ -16,10 +16,10 @@
>>
>> #define PMIC_GLINK_MAX_PORTS 3
>>
>> -#define UCSI_BUF_SIZE 48
>> +#define UCSI_BUF_V1_SIZE (UCSI_MESSAGE_OUT + (UCSI_MESSAGE_OUT - UCSI_MESSAGE_IN))
>> +#define UCSI_BUF_V2_SIZE (UCSIv2_MESSAGE_OUT + (UCSIv2_MESSAGE_OUT - UCSI_MESSAGE_IN))
>>
>> #define MSG_TYPE_REQ_RESP 1
>> -#define UCSI_BUF_SIZE 48
>>
>> #define UC_NOTIFY_RECEIVER_UCSI 0x0
>> #define UC_UCSI_READ_BUF_REQ 0x11
>> @@ -32,13 +32,25 @@ struct ucsi_read_buf_req_msg {
>>
>> struct ucsi_read_buf_resp_msg {
>> struct pmic_glink_hdr hdr;
>> - u8 buf[UCSI_BUF_SIZE];
>> + u8 buf[UCSI_BUF_V1_SIZE];
>> + u32 ret_code;
>
> What is the endian-ness of ret_code as it comes from the device?
ret_code is little endian but since ret_code = 0 on success we only ever
check " if (ret_code)" and return early if there is any error.
>> +};
>> +
>> +struct ucsi_v2_read_buf_resp_msg {
>> + struct pmic_glink_hdr hdr;
>> + u8 buf[UCSI_BUF_V2_SIZE];
>> u32 ret_code;
>> };
>>
>> struct ucsi_write_buf_req_msg {
>> struct pmic_glink_hdr hdr;
>> - u8 buf[UCSI_BUF_SIZE];
>> + u8 buf[UCSI_BUF_V1_SIZE];
>> + u32 reserved;
>
> What is "reserved" for? What is it set to? Where is it tested to
> ensure it is set properly?
The reserved property is part of the message format defined by the
charger FW(PPM). We do not set it explicitly but I will add length
checks to our write requests to ensure we do not write to this property.
>
>> +};
>> +
>> +struct ucsi_v2_write_buf_req_msg {
>> + struct pmic_glink_hdr hdr;
>> + u8 buf[UCSI_BUF_V2_SIZE];
>> u32 reserved;
>> };
>
> And for all of these structures, are you sure there are not "holes" in
> them? As you are using sizeof(), don't they need to be packed?
>
Ack - will update to be packed
>>
>> @@ -72,7 +84,7 @@ struct pmic_glink_ucsi {
>> bool ucsi_registered;
>> bool pd_running;
>>
>> - u8 read_buf[UCSI_BUF_SIZE];
>> + u8 read_buf[UCSI_BUF_V2_SIZE];
>
> Why is this one just v2 and not also v1?
Read_buf[] is used to read back the read_requests. If we added a
separate read buffer for v2 we would need to add more logic into
pmic_glink_ucsi_read() and pmic_glink_ucsi_read_ack(). By setting
read_buf[] to the maximum buffer size (v2) then we can reuse this buffer
for v1 and v2 and code reuse will be cleaner.
>
>> };
>>
>> static int pmic_glink_ucsi_read(struct ucsi *__ucsi, unsigned int offset,
>> @@ -131,8 +143,9 @@ static int pmic_glink_ucsi_read_message_in(struct ucsi *ucsi, void *val, size_t
>> static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned int offset,
>> const void *val, size_t val_len)
>> {
>> - struct ucsi_write_buf_req_msg req = {};
>> + struct ucsi_v2_write_buf_req_msg req = {};
>> unsigned long left;
>> + size_t len;
>> int ret;
>>
>> req.hdr.owner = PMIC_GLINK_OWNER_USBC;
>> @@ -142,7 +155,18 @@ static int pmic_glink_ucsi_locked_write(struct pmic_glink_ucsi *ucsi, unsigned i
>>
>> reinit_completion(&ucsi->write_ack);
>>
>> - ret = pmic_glink_send(ucsi->client, &req, sizeof(req));
>> + if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
>> + /* If UCSI version is unknown, use the maximum buffer size */
>> + len = sizeof(req);
>> + } else {
>> + /*
>> + * If UCSI V1, buffer size should be UCSI_BUF_V1_SIZE so update
>> + * len accordingly
>> + */
>> + len = sizeof(struct ucsi_write_buf_req_msg);
>> + }
>> +
>> + ret = pmic_glink_send(ucsi->client, &req, len);
>> if (ret < 0) {
>> dev_err(ucsi->dev, "failed to send UCSI write request: %d\n", ret);
>> return ret;
>> @@ -216,12 +240,26 @@ static const struct ucsi_operations pmic_glink_ucsi_ops = {
>>
>> static void pmic_glink_ucsi_read_ack(struct pmic_glink_ucsi *ucsi, const void *data, int len)
>> {
>> - const struct ucsi_read_buf_resp_msg *resp = data;
>> + const struct ucsi_v2_read_buf_resp_msg *resp = data;
>> + u32 ret_code, buffer_len;
>> +
>> + if (!ucsi->ucsi->version || ucsi->ucsi->version >= UCSI_VERSION_2_1) {
>> + /* If UCSI version is unknown, use the maximum buffer size */
>> + ret_code = resp->ret_code;
>
> No endian change?
We only check if ret_code == 0, so no need for endian change
>
>> + buffer_len = UCSI_BUF_V2_SIZE;
>
> Shouldn't you warn about this? Doesn't the version have to be set?
Version is set from a read_request during ucsi_register(). This would be
the first read_request we receive and should be the only time we do not
know the version. Will add the ucsi->ucsi_registered var to enforce that
version is unknown ONLY before ucsi_register() otherwise warn/return error.
>
>
>> + } else {
>> + /*
>> + * If UCSI V1, use UCSI_BUF_V1_SIZE buffer size and
>> + * update ret_code offset accordingly
>> + */
>> + ret_code = ((struct ucsi_read_buf_resp_msg *)data)->ret_code;
>
> Are you sure the buffer was that big? Where was that checked?
ACK - will add length checks
>
>> + buffer_len = UCSI_BUF_V1_SIZE;
>> + }
>>
>> - if (resp->ret_code)
>> + if (ret_code)
>> return;
>>
>> - memcpy(ucsi->read_buf, resp->buf, UCSI_BUF_SIZE);
>> + memcpy(ucsi->read_buf, resp->buf, buffer_len);
>
> Where is the length checked against the "real" length of the buffer
> passed in here? What happens if the device lies?
>
Ack - will add length checks
> thanks,
>
> greg k-h
Powered by blists - more mailing lists