On Tuesday 12 November 2019 00:16:59 CET Al Viro wrote:
[...]
> More fun:
> int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id, void *val, size_t val_len)
> {
> int ret;
> struct hif_msg *hif;
> int buf_len = sizeof(struct hif_cnf_read_mib) + val_len;
> struct hif_req_read_mib *body = wfx_alloc_hif(sizeof(*body), &hif);
> struct hif_cnf_read_mib *reply = kmalloc(buf_len, GFP_KERNEL);
>
> OK, allocated request and reply buffers, by the look of it; request one
> being struct hif_msg with struct hif_req_read_mib for payload
> and reply - struct hif_cnf_read_mib {
> uint32_t status;
> uint16_t mib_id;
> uint16_t length;
> uint8_t mib_data[];
> } with val_len bytes in mib_data.
>
> body->mib_id = cpu_to_le16(mib_id);
> wfx_fill_header(hif, vif_id, HIF_REQ_ID_READ_MIB, sizeof(*body));
>
> Filled request, {.len = cpu_to_le16(4 + 4),
> .id = HIF_REQ_ID_READ_MIB,
> .interface = vif_id,
> .body = {
> .mib_id = cpu_to_le16(mib_id)
> }
> }
> Note that mib_id is host-endian here; what we send is little-endian.
>
> ret = wfx_cmd_send(wdev, hif, reply, buf_len, false);
> send it, get reply
>
> if (!ret && mib_id != reply->mib_id) {
> Wha...? Now we are comparing two bytes at offset 4 into reply with a host-endian
> value? Oh, well...
Agree.
>
> dev_warn(wdev->dev, "%s: confirmation mismatch request\n", __func__);
> ret = -EIO;
> }
> if (ret == -ENOMEM)
> dev_err(wdev->dev, "buffer is too small to receive %s (%zu < %d)\n",
> get_mib_name(mib_id), val_len, reply->length);
> if (!ret)
> memcpy(val, &reply->mib_data, reply->length);
> What. The. Hell?
>
> We are copying data from the reply. Into caller-supplied object.
> With length taken from the same reply and no validation even
> attempted? Not even "um, maybe we shouldn't copy more than the caller
> told us to copy, especially since that's as much as there is in the
> source of that memcpy"?
In fact, hif_generic_confirm() check that data from hardware is smaller
than "buf_len". If it is not the case, ret will contains -ENOMEM. But
indeed, if size of data is correct but reply->length is corrupted, we
will have big trouble.
(In add, I am not sure that -ENOMEM is well chosen for this case)
> And that's besides the endianness questions. Note that getting the
> endianness wrong here is just about certain to blow up - small value
> will be misinterpreted by factor of 256.
>
> In any case, even if this is talking to firmware on a card, that's
> an unhealthy degree of trust, especially since the same function
> does consider the possibility of bogus replies.
It is obvious that the errors paths have not been sufficiently checked.
If you continue to search, I think you will find many similar problems.
I will update the TODO list attached to the driver.
--
Jérôme Pouiller
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
