| 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: <8738628a5c1b87c6521fdd8d05a3b36e5c32b48a.camel@hexdev.de>
Date: Sat, 11 May 2024 10:14:46 +0200
From: Christoph Fritz <christoph.fritz@...dev.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Jiri Slaby <jirislaby@...nel.org>, Simon Horman <horms@...nel.org>, Greg
Kroah-Hartman <gregkh@...uxfoundation.org>, Marc Kleine-Budde
<mkl@...gutronix.de>, Oliver Hartkopp <socketcan@...tkopp.net>, Vincent
Mailhol <mailhol.vincent@...adoo.fr>, "David S . Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jiri Kosina <jikos@...nel.org>, Benjamin Tissoires
<bentiss@...nel.org>, Sebastian Reichel <sre@...nel.org>, Linus Walleij
<linus.walleij@...aro.org>, Andreas Lauser
<andreas.lauser@...cedes-benz.com>, Jonathan Corbet <corbet@....net>, Pavel
Pisa <pisa@....felk.cvut.cz>, linux-can@...r.kernel.org, Netdev
<netdev@...r.kernel.org>, devicetree@...r.kernel.org,
linux-input@...r.kernel.org, linux-serial <linux-serial@...r.kernel.org>
Subject: Re: [PATCH v4 02/11] HID: hexLIN: Add support for USB LIN adapter
...
> > diff --git a/drivers/hid/hid-hexdev-hexlin.c b/drivers/hid/hid-hexdev-hexlin.c
> > new file mode 100644
> > index 0000000000000..a9ed080b3e33e
> > --- /dev/null
> > +++ b/drivers/hid/hid-hexdev-hexlin.c
> >
...
>
> > +}
> > +
> > +#define HEXLIN_GET_CMD(name, enum_cmd) \
> > + static int hexlin_##name(struct hexlin_priv_data *p) \
> > + { \
> > + u8 *req; \
> > + int ret; \
> > + \
> > + req = kmalloc(sizeof(*req), GFP_KERNEL) ; \
>
> Extra space.
>
> Use:
>
> u8 *req __free(kfree) = kmalloc(sizeof(*req), GFP_KERNEL);
>
> > + if (!req) \
> > + return -ENOMEM; \
> > + \
> > + *req = enum_cmd; \
> > + \
> > + ret = hexlin_tx_req_status(p, req, sizeof(*req)); \
> > + if (ret) \
> > + hid_err(p->hid_dev, "%s failed, error: %d\n", \
> > + #name, ret); \
> > + \
> > + kfree(req); \
>
> Not needed after using __free().
>
> > + return ret; \
> > + }
> > +
> > +HEXLIN_GET_CMD(get_version, HEXLIN_GET_VERSION)
> > +HEXLIN_GET_CMD(reset_dev, HEXLIN_RESET)
> > +HEXLIN_GET_CMD(get_baudrate, HEXLIN_GET_BAUDRATE)
>
> Could you convert the function in the macro into a helper function which
> is just called by a simple function with the relevant parameters for
> these 3 cases?
The device actually has a lot more features that I'm using in my debug
version and which might end up here in the future. So I would like to
keep it. Any objections?
...
> > +
> > +static int hexlin_set_baudrate(struct hexlin_priv_data *priv, u16 baudrate)
> > +{
> > + struct hexlin_baudrate_req *req;
> > + int ret;
> > +
> > + if (baudrate < LIN_MIN_BAUDRATE || baudrate > LIN_MAX_BAUDRATE)
> > + return -EINVAL;
> > +
> > + req = kmalloc(sizeof(*req), GFP_KERNEL);
>
> Hmm... Why do you alloc this small structure (3 bytes?) with kmalloc() and
> not just have it in stack as a local variable?
This buffer must be suitable for DMA (see docu for struct urb).
So with a stack variable we would need to use kmemdup() before the
actual sending call, but that's what you did not like since v3 so I
changed it to this which now you also don't like.
Let's dial it back to the original kmemdup() usage, ok?
...
> > +static int hexlin_queue_frames_insert(struct hexlin_priv_data *priv,
> > + const struct hexlin_frame *hxf)
> > +{
> > + struct hid_device *hdev = priv->hid_dev;
> > + struct lin_frame lf;
> > +
> > + lf.len = hxf->len;
> > + lf.lin_id = hxf->lin_id;
> > + memcpy(lf.data, hxf->data, LIN_MAX_DLEN);
> > + lf.checksum = hxf->checksum;
> > + lf.checksum_mode = hxf->checksum_mode;
> > +
> > + hid_dbg(hdev, "id:%02x, len:%u, data:%*ph, chk:%02x (%s), flg:%08x\n",
> > + lf.lin_id, lf.len, lf.len, lf.data, lf.checksum,
> > + lf.checksum_mode ? "enhanced" : "classic", hxf->flags);
> > +
> > + lin_rx(priv->ldev, &lf);
> > +
> > + return 0;
> > +}
> > +
> > +static int hexlin_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data, int sz)
> > +{
> > + struct hexlin_priv_data *priv;
> > + struct hexlin_baudrate_req *br;
> > + struct hexlin_responder_answer_req *rar;
> > + struct hexlin_unconditional_req *hfr;
> > + struct hexlin_val8_req *vr;
> > +
> > + if (sz < 1 || sz > HEXLIN_PKGLEN_MAX)
> > + return -EREMOTEIO;
> > +
> > + priv = hid_get_drvdata(hdev);
> > +
> > + hid_dbg(hdev, "%s, size:%i, data[0]: 0x%02x\n", __func__, sz, data[0]);
> > +
> > + priv->is_error = false;
> > +
> > + switch (data[0]) {
> > + case HEXLIN_SUCCESS:
> > + if (sz != HEXLIN_LEN_RETCODE)
> > + return -EREMOTEIO;
> > + hid_dbg(hdev, "HEXLIN_SUCCESS: 0x%02x\n", data[0]);
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_FAIL:
> > + if (sz != HEXLIN_LEN_RETCODE)
> > + return -EREMOTEIO;
> > + hid_err(hdev, "HEXLIN_FAIL: 0x%02x\n", data[0]);
> > + priv->is_error = true;
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_VERSION:
> > + if (sz != sizeof(*vr))
> > + return -EREMOTEIO;
> > + vr = (struct hexlin_val8_req *) data;
> > + priv->fw_version = vr->v;
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_RESPONDER_ANSWER_ID:
> > + if (sz != sizeof(*rar))
> > + return -EREMOTEIO;
> > + rar = (struct hexlin_responder_answer_req *) data;
> > + memcpy(&priv->answ, &rar->answ, sizeof(priv->answ));
> > + complete(&priv->wait_in_report);
> > + break;
> > + case HEXLIN_GET_BAUDRATE:
> > + if (sz != sizeof(*br))
> > + return -EREMOTEIO;
> > + br = (struct hexlin_baudrate_req *) data;
> > + le16_to_cpus(br->baudrate);
> > + priv->baudrate = br->baudrate;
> > + complete(&priv->wait_in_report);
> > + break;
> > + /* following cases not initiated by us, so no complete() */
> > + case HEXLIN_FRAME:
> > + if (sz != sizeof(*hfr)) {
> > + hid_err_once(hdev, "frame size mismatch: %i\n", sz);
> > + return -EREMOTEIO;
> > + }
> > + hfr = (struct hexlin_unconditional_req *) data;
> > + le32_to_cpus(hfr->frm.flags);
>
> I'm bit worried about this from endianness perspective. Perhaps there's
> some struct reusing that shouldn't be happening because the same field
> cannot be __le32 and u32 at the same time.
Can you propose a solution?
>
...
thanks
-- Christoph
Powered by blists - more mailing lists