Re: [Qemu-devel] [Qemu-block] [PATCH 09/10] scsi: add multipath support to qemu-pr-helper

[Paolo Bonzini <pbonzini@redhat.com>]

On 30/08/2017 18:37, Stefan Hajnoczi wrote:
> 
> The case statements asssume sz has a certain minimum value.  I didn't
> see a check anywhere that guarantees this.  It may be easier to hide the
> client's sz value and instead use sizeof(client->data).  The caller can
> worry about sz.

Makes sense.  OUT needs the client sz, but IN doesn't and it gets in the
way.  This lets me just assert in multipath_pr_in that sz is large enough.

>> +    /* Convert input data, especially transport IDs, to the structs
>> +     * used by libmpathpersist (which, of course, will immediately
>> +     * do the opposite).
>> +     */
>> +    memset(&paramp, 0, sizeof(paramp));
>> +    memcpy(&paramp.key, &param[0], 8);
>> +    memcpy(&paramp.sa_key, &param[8], 8);
>> +    paramp.sa_flags = param[10];
>> +    for (i = PR_OUT_FIXED_PARAM_SIZE, j = 0; i < sz; ) {
>> +        struct transportid *id = (struct transportid *) &transportids[j];
>> +        int len;
>> +
>> +        id->format_code = param[i] & 0xc0;
>> +        id->protocol_id = param[i] & 0x0f;
>> +        switch (param[i] & 0xcf) {
> At this point we know sz > PR_OUT_FIXED_PARAM_SIZE && i < sz.  I think
> the following case statements can read beyond the end of client->data[]
> because nothing checks sz before accessing param[].
> 
> Missing sz checks?

There is a transport id length field that has to be checked against sz,
indeed.  After doing that, the for loop is fine (though the initial
index is wrong, because PR_OUT_FIXED_PARAM_SIZE points to the length
field and the transport ids are at PR_OUT_FIXED_PARAM_SIZE + 4).

>> +            /* iSCSI transport.  */
>> +            len = lduw_be_p(&param[i + 2]);
>> +            if (len > 252 || (len & 3)) {
> 
> int len can be negative here .  Please use the size_t type - it's
> unsigned and used by memchr(3)/memcpy(3).

Can it? lduw_be_p reads 16 bits (and it's unsigned as the name says).

Paolo