[LEDE-DEV] Fix for uqmi crash when using qmi-via-mbim (--mbim / -m)

[Mogens Lauridsen]

Seems like a better fix, but it doesn't work. uqmi hangs, so I suspect
that EM7455 has misunderstood the command. I have removed/replaced the
ustream_write(..,..,.., true) and after the changes below it works.
I guess it has something to do with the write being split in two.
I don't know what the maximum size of buffer should be, so I used:
2048+sizeof(struct mbim_command_message)


--- dev.c_    2016-11-22 13:49:36.953419346 +0100
+++ dev.c    2016-11-22 14:03:27.089424031 +0100
@@ -161,9 +161,10 @@

 int qmi_request_start(struct qmi_dev *qmi, struct qmi_request *req,
struct qmi_msg *msg, request_cb cb)
 {
+    char buf[2048+sizeof(struct mbim_command_message)];
+    int buf_len = 0;
     int len = qmi_complete_request_message(msg);
     uint16_t tid;
-    char *buf = (void *) msg;

     memset(req, 0, sizeof(*req));
     req->ret = -1;
@@ -188,13 +189,14 @@
     list_add(&req->list, &qmi->req);

     if (qmi->is_mbim) {
-        struct mbim_command_message mbim;
-        mbim_qmi_cmd(&mbim, len, tid);
-        ustream_write(&qmi->sf.stream, (void *) &mbim, sizeof(mbim), true);
+        mbim_qmi_cmd((struct mbim_command_message *)buf, len, tid);
+        buf_len += sizeof(struct mbim_command_message);
     }
+    memcpy(buf + buf_len, msg, len);
+    buf_len += len;

-    dump_packet("Send packet", buf, len);
-    ustream_write(&qmi->sf.stream, buf, len, false);
+    dump_packet("Send packet", buf, buf_len);
+    ustream_write(&qmi->sf.stream, buf, buf_len, false);
     return 0;
 }

On Tue, Nov 22, 2016 at 1:12 PM, Felix Fietkau <nbd at nbd.name> wrote:
> On 2016-11-22 12:47, Mogens Lauridsen wrote:
>> Hi,
>>
>> I found a memory overwrite causing a crash when using uqmi and
>> qmi-via-mbim such as:
>> uqmi -m -d /dev/cdc-wdm0 --get-signal-info
>>
>> The problem is missing space for mbim header, which is assumed in
>> qmi_request_start():
>>
>> if (qmi->is_mbim) {
>>                  buf -= sizeof(struct mbim_command_message);
>>
>> I have fixed it by added a new buffer "buf_" and set the original "buf"
>> to point inside "buf_"
> I have a better fix in mind. Please try this:
>
> diff --git a/dev.c b/dev.c
> index 9bf7ab2..9662a9a 100644
> --- a/dev.c
> +++ b/dev.c
> @@ -38,11 +38,8 @@ static const uint8_t qmi_services[__QMI_SERVICE_LAST] = {
>  #undef __qmi_service
>
>  static struct {
> -       struct mbim_command_message mbim;
> -       union {
> -               char buf[512];
> -               struct qmi_msg msg;
> -       } u;
> +       char buf[512];
> +       struct qmi_msg msg;
>  } __packed msgbuf;
>
>  #ifdef DEBUG_PACKET
> @@ -191,9 +188,9 @@ int qmi_request_start(struct qmi_dev *qmi, struct qmi_request *req, struct qmi_m
>         list_add(&req->list, &qmi->req);
>
>         if (qmi->is_mbim) {
> -               buf -= sizeof(struct mbim_command_message);
> -               mbim_qmi_cmd((struct mbim_command_message *) buf, len, tid);
> -               len += sizeof(struct mbim_command_message);
> +               struct mbim_command_message mbim;
> +               mbim_qmi_cmd(&mbim, len, tid);
> +               ustream_write(&qmi->sf.stream, (void *) &mbim, sizeof(mbim), true);
>         }
>
>         dump_packet("Send packet", buf, len);
> @@ -260,7 +257,7 @@ int qmi_service_connect(struct qmi_dev *qmi, QmiService svc, int client_id)
>         };
>         struct qmi_connect_request req;
>         int idx = qmi_get_service_idx(svc);
> -       struct qmi_msg *msg = &msgbuf.u.msg;
> +       struct qmi_msg *msg = &msgbuf.msg;
>
>         if (idx < 0)
>                 return -1;
> @@ -299,7 +296,7 @@ static void __qmi_service_disconnect(struct qmi_dev *qmi, int idx)
>                 )
>         };
>         struct qmi_request req;
> -       struct qmi_msg *msg = &msgbuf.u.msg;
> +       struct qmi_msg *msg = &msgbuf.msg;
>
>         qmi->service_connected &= ~(1 << idx);
>         qmi->service_data[idx].client_id = -1;
>