linux-kernel - Re: [PATCH v4 5/7] platform/chrome: cros_ec

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANFp7mV75SpTAyh=A4kZA2=NWi7+ry0+W+a5kpcjLrPwyhJ8UQ@mail.gmail.com>
Date: Fri, 13 Dec 2024 10:33:19 -0800
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Stephen Boyd <sboyd@...nel.org>
Cc: chrome-platform@...ts.linux.dev, heikki.krogerus@...ux.intel.com, 
	linux-usb@...r.kernel.org, tzungbi@...nel.org, akuchynski@...gle.com, 
	pmalani@...omium.org, jthies@...gle.com, dmitry.baryshkov@...aro.org, 
	badhri@...gle.com, rdbabiera@...gle.com, Benson Leung <bleung@...omium.org>, 
	Guenter Roeck <groeck@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/7] platform/chrome: cros_ec_typec: Displayport support

On Wed, Dec 11, 2024 at 1:58 PM Stephen Boyd <sboyd@...nel.org> wrote:
>
> Quoting Abhishek Pandit-Subedi (2024-12-06 15:38:16)
> > diff --git a/drivers/platform/chrome/cros_typec_altmode.c b/drivers/platform/chrome/cros_typec_altmode.c
> > new file mode 100644
> > index 000000000000..bb7c7ad2ff6e
> > --- /dev/null
> > +++ b/drivers/platform/chrome/cros_typec_altmode.c
> > @@ -0,0 +1,281 @@
> [...]
> > +
> > +static const struct typec_altmode_ops cros_typec_altmode_ops = {
> > +       .enter = cros_typec_altmode_enter,
> > +       .exit = cros_typec_altmode_exit,
> > +       .vdm = cros_typec_altmode_vdm,
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_TYPEC_DP_ALTMODE)
> > +int cros_typec_displayport_status_update(struct typec_altmode *altmode,
> > +                                        struct typec_displayport_data *data)
> > +{
> > +       struct cros_typec_dp_data *dp_data =
> > +               typec_altmode_get_drvdata(altmode);
>
> How does this work? I see that the type of the drvdata is struct
> cros_typec_altmode_data per the allocation in
> cros_typec_register_displayport(), but here we're treating it as the
> type struct cros_typec_dp_data, which has a struct
> cros_typec_altmode_data as the first member. The allocation is too small
> from what I can tell. The same problem looks to be there in
> cros_typec_displayport_vdm().
>
> > +       struct cros_typec_altmode_data *adata = &dp_data->adata;
> > +
> > +       if (!dp_data->pending_status_update) {
> > +               dev_dbg(&altmode->dev,
> > +                       "Got DPStatus without a pending request");
> > +               return 0;
> > +       }
> > +
> > +       if (dp_data->configured && dp_data->data.conf != data->conf)
> > +               dev_dbg(&altmode->dev,
> > +                       "DP Conf doesn't match. Requested 0x%04x, Actual 0x%04x",
> > +                       dp_data->data.conf, data->conf);
> > +
> > +       mutex_lock(&adata->lock);
> > +
> > +       dp_data->data = *data;
> > +       dp_data->pending_status_update = false;
> > +       adata->header |= VDO_CMDT(CMDT_RSP_ACK);
> > +       adata->vdo_data = &dp_data->data.status;
> > +       adata->vdo_size = 2;
> > +       schedule_work(&adata->work);
> > +
> > +       mutex_unlock(&adata->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +struct typec_altmode *
> > +cros_typec_register_displayport(struct cros_typec_port *port,
> > +                               struct typec_altmode_desc *desc,
> > +                               bool ap_mode_entry)
> > +{
> > +       struct typec_altmode *alt;
> > +       struct cros_typec_altmode_data *data;
> > +
> > +       alt = typec_port_register_altmode(port->port, desc);
> > +       if (IS_ERR(alt))
> > +               return alt;
> > +
> > +       data = devm_kzalloc(&alt->dev, sizeof(*data), GFP_KERNEL);
> > +       if (!data) {
> > +               typec_unregister_altmode(alt);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> > +       INIT_WORK(&data->work, cros_typec_altmode_work);
> > +       mutex_init(&data->lock);
> > +       data->alt = alt;
> > +       data->port = port;
> > +       data->ap_mode_entry = ap_mode_entry;
> > +       data->sid = desc->svid;
> > +       data->mode = desc->mode;
> > +
> > +       typec_altmode_set_ops(alt, &cros_typec_altmode_ops);
> > +       typec_altmode_set_drvdata(alt, data);
>
> 'data' is of type struct cros_typec_altmode_data here
This should have been allocated as cros_typec_dp_data. Missed during a
previous refactor that changed the type from a union to this format.

>
> > +
> > +       return alt;
> > +}