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

[Bjørn Mork]

Felix Fietkau <nbd at nbd.name> writes:

> 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);
>  	}


Yes, looks much nicer, but does not work for me unfortunately.  Tested
with an EM7455.  It returns

  Received packet: 04 00 00 80 10 00 00 00 01 00 00 00 03 00 00 00

which translates to MBIM_ERROR_LENGTH_MISMATCH. Looking at the usbmon
dump I see the explanation: The ustream_write() causes an immediate
write, splitting the MBIM message in two writes.  This is not supported
by the driver.  Definitely a little underdocumented, I'll admit...

But whatever fix we make will need to merge the mbim header and the qmi
message before writing it to the /dev/cdc-wdmX device.

BTW, thanks to Mogens for finding this bug and sorry for the sloppy
initial implementation.



Bjørn