Re: [PATCH] staging: wfx: add gcc extension __force cast

[Jerome Pouiller]

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