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