[v4,1/3] usb: typec: ucsi: Limit read size on v1.2
Commit Message
Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
increased from 16 to 256. In order to avoid overflowing reads for older
systems, add a mechanism to use the read UCSI version to truncate read
sizes on UCSI v1.2.
Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Prashant Malani <pmalani@chromium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Tested on 6.6 kernel. Dmesg output from this change:
[ 105.058162] ucsi_um_test ucsi_um_test_device.0: Registered UCSI
interface with version 3.0.0
Changes in v4:
- Added missing Tested-By tags from v1 and reviewed-by tags.
Changes in v2:
- Changed log message to DEBUG
drivers/usb/typec/ucsi/ucsi.c | 26 ++++++++++++++++++++++++--
drivers/usb/typec/ucsi/ucsi.h | 11 +++++++++++
2 files changed, 35 insertions(+), 2 deletions(-)
Comments
On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:
> Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
> increased from 16 to 256. In order to avoid overflowing reads for older
> systems, add a mechanism to use the read UCSI version to truncate read
> sizes on UCSI v1.2.
..
> + if (ucsi->version <= UCSI_VERSION_1_2)
> + buf_size = min_t(size_t, 16, buf_size);
Please, avoid using min_t(). Here the clamp() can be used.
Shouldn't magic number be defined?
On Fri, Feb 9, 2024 at 6:28 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:
> > Between UCSI 1.2 and UCSI 2.0, the size of the MESSAGE_IN region was
> > increased from 16 to 256. In order to avoid overflowing reads for older
> > systems, add a mechanism to use the read UCSI version to truncate read
> > sizes on UCSI v1.2.
>
> ...
>
> > + if (ucsi->version <= UCSI_VERSION_1_2)
> > + buf_size = min_t(size_t, 16, buf_size);
>
> Please, avoid using min_t(). Here the clamp() can be used.
I think this is likely the 4th time I've been tripped up by an
undocumented practice in this patch series. <linux/minmax.h> says
nothing about avoiding min_t -- why prefer clamp()? Please add the
recommendation here
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/minmax.h#n10)
and I am more than happy to change it after.
> Shouldn't magic number be defined?
The comment right above this line documents the number.
As this is the only use right now, I don't see a need to make it a
macro/constant yet.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Cheers,
Abhishek
On Fri, Feb 09, 2024 at 10:01:07AM -0800, Abhishek Pandit-Subedi wrote:
> On Fri, Feb 9, 2024 at 6:28 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:
..
> > > + if (ucsi->version <= UCSI_VERSION_1_2)
> > > + buf_size = min_t(size_t, 16, buf_size);
> >
> > Please, avoid using min_t(). Here the clamp() can be used.
> I think this is likely the 4th time I've been tripped up by an
> undocumented practice in this patch series. <linux/minmax.h> says
> nothing about avoiding min_t -- why prefer clamp()?
While in this case it will work correctly, the size_t is unsigned type and 16
is signed, while buf_size is unknown in this context. It means if buf_size is
signed, the min_t gives wrong result. clamp() is better choice.
See also, e.g., https://lore.kernel.org/all/20231004064220.31452-1-biju.das.jz@bp.renesas.com/.
> Please add the
> recommendation here
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/minmax.h#n10)
> and I am more than happy to change it after.
It's not my recommendation :-)
https://lore.kernel.org/all/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/
Feel free to submit a patch.
..
> > Shouldn't magic number be defined?
> The comment right above this line documents the number.
> As this is the only use right now, I don't see a need to make it a
> macro/constant yet.
OK.
On Fri, Feb 9, 2024 at 11:39 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Feb 09, 2024 at 10:01:07AM -0800, Abhishek Pandit-Subedi wrote:
> > On Fri, Feb 9, 2024 at 6:28 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Feb 08, 2024 at 10:02:38PM -0800, Abhishek Pandit-Subedi wrote:
>
> ...
>
> > > > + if (ucsi->version <= UCSI_VERSION_1_2)
> > > > + buf_size = min_t(size_t, 16, buf_size);
> > >
> > > Please, avoid using min_t(). Here the clamp() can be used.
> > I think this is likely the 4th time I've been tripped up by an
> > undocumented practice in this patch series. <linux/minmax.h> says
> > nothing about avoiding min_t -- why prefer clamp()?
>
> While in this case it will work correctly, the size_t is unsigned type and 16
> is signed, while buf_size is unknown in this context. It means if buf_size is
> signed, the min_t gives wrong result. clamp() is better choice.
>
> See also, e.g., https://lore.kernel.org/all/20231004064220.31452-1-biju.das.jz@bp.renesas.com/.
>
> > Please add the
> > recommendation here
> > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/minmax.h#n10)
> > and I am more than happy to change it after.
>
> It's not my recommendation :-)
>
> https://lore.kernel.org/all/CAHk-=whwEAc22wm8h9FESPB5X+P4bLDgv0erBQMa1buTNQW7tA@mail.gmail.com/
>
> Feel free to submit a patch.
>
Ack, will send up a PATCH v5. And add to my backlog of foot-gun
checkpatch and docs fixes I need to send up :)
> ...
>
> > > Shouldn't magic number be defined?
> > The comment right above this line documents the number.
> > As this is the only use right now, I don't see a need to make it a
> > macro/constant yet.
>
> OK.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
@@ -36,6 +36,19 @@
*/
#define UCSI_SWAP_TIMEOUT_MS 5000
+static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
+ size_t buf_size)
+{
+ /*
+ * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
+ * reads here.
+ */
+ if (ucsi->version <= UCSI_VERSION_1_2)
+ buf_size = min_t(size_t, 16, buf_size);
+
+ return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
+}
+
static int ucsi_acknowledge_command(struct ucsi *ucsi)
{
u64 ctrl;
@@ -72,7 +85,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
if (ret < 0)
return ret;
- ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, &error, sizeof(error));
+ ret = ucsi_read_message_in(ucsi, &error, sizeof(error));
if (ret)
return ret;
@@ -170,7 +183,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
length = ret;
if (data) {
- ret = ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, data, size);
+ ret = ucsi_read_message_in(ucsi, data, size);
if (ret)
goto out;
}
@@ -1558,6 +1571,15 @@ int ucsi_register(struct ucsi *ucsi)
if (!ucsi->version)
return -ENODEV;
+ /*
+ * Version format is JJ.M.N (JJ = Major version, M = Minor version,
+ * N = sub-minor version).
+ */
+ dev_dbg(ucsi->dev, "Registered UCSI interface with version %x.%x.%x",
+ UCSI_BCD_GET_MAJOR(ucsi->version),
+ UCSI_BCD_GET_MINOR(ucsi->version),
+ UCSI_BCD_GET_SUBMINOR(ucsi->version));
+
queue_delayed_work(system_long_wq, &ucsi->work, 0);
ucsi_debugfs_register(ucsi);
@@ -23,6 +23,17 @@ struct dentry;
#define UCSI_CONTROL 8
#define UCSI_MESSAGE_IN 16
#define UCSI_MESSAGE_OUT 32
+#define UCSIv2_MESSAGE_OUT 272
+
+/* UCSI versions */
+#define UCSI_VERSION_1_2 0x0120
+#define UCSI_VERSION_2_0 0x0200
+#define UCSI_VERSION_2_1 0x0210
+#define UCSI_VERSION_3_0 0x0300
+
+#define UCSI_BCD_GET_MAJOR(_v_) (((_v_) >> 8) & 0xFF)
+#define UCSI_BCD_GET_MINOR(_v_) (((_v_) >> 4) & 0x0F)
+#define UCSI_BCD_GET_SUBMINOR(_v_) ((_v_) & 0x0F)
/* Command Status and Connector Change Indication (CCI) bits */
#define UCSI_CCI_CONNECTOR(_c_) (((_c_) & GENMASK(7, 1)) >> 1)