[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 09/10] scsi: add multipath support
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 09/10] scsi: add multipath support to qemu-pr-helper |
Date: |
Mon, 11 Sep 2017 11:14:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 |
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(¶mp, 0, sizeof(paramp));
>> + memcpy(¶mp.key, ¶m[0], 8);
>> + memcpy(¶mp.sa_key, ¶m[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(¶m[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
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-block] [Qemu-devel] [PATCH 09/10] scsi: add multipath support to qemu-pr-helper,
Paolo Bonzini <=