mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

pr-helper: avoid error on PR IN command with zero request size

Browse Source 
After reading a PR IN command with zero request size in prh_read_request,
the resp->result field will be uninitialized and the resp.sz field will
be also uninitialized when returning to prh_co_entry.

If resp->result == GOOD (from a previous successful reply or just luck),
then the assert in prh_write_response might not be triggered and
uninitialized response will be sent.

The fix is to remove the whole handling of sz == 0 in prh_co_entry.
Those errors apply only to PR OUT commands and it's perfectly okay to
catch them later in do_pr_out and multipath_pr_out; the check for
too-short parameters in fact doesn't apply in the easy SG_IO case, as
it can be left to the target firmware even.

The result is that prh_read_request does not fail requests anymore and
prh_co_entry becomes simpler.

Reported-by: Dima Stepanov <dimastep@yandex-team.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This commit is contained in:
Paolo Bonzini 2018-07-02 14:21:58 +02:00
parent 2a018f6e98
commit ee8c13b814
1 changed files with 30 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

63
scsi/qemu-pr-helper.c
View File

@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
char transportids[PR_HELPER_DATA_SIZE]; char transportids[PR_HELPER_DATA_SIZE];
int r; int r;
if (sz < PR_OUT_FIXED_PARAM_SIZE) {
/* Illegal request, Parameter list length error. This isn't fatal;
* we have read the data, send an error without closing the socket.
*/
scsi_build_sense(sense, SENSE_CODE(INVALID_PARAM_LEN));
return CHECK_CONDITION;
}
switch (rq_servact) { switch (rq_servact) {
case MPATH_PROUT_REG_SA: case MPATH_PROUT_REG_SA:
case MPATH_PROUT_RES_SA: case MPATH_PROUT_RES_SA:
@ -574,6 +582,12 @@ static int do_pr_out(int fd, const uint8_t *cdb, uint8_t *sense,
const uint8_t *param, int sz) const uint8_t *param, int sz)
{ {
int resp_sz; int resp_sz;
if ((fcntl(fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
scsi_build_sense(sense, SENSE_CODE(INVALID_OPCODE));
return CHECK_CONDITION;
}
#ifdef CONFIG_MPATH #ifdef CONFIG_MPATH
if (is_mpath(fd)) { if (is_mpath(fd)) {
return multipath_pr_out(fd, cdb, sense, param, sz); return multipath_pr_out(fd, cdb, sense, param, sz);
@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient *client,
errp) < 0) { errp) < 0) {
goto out_close; goto out_close;
} }
if ((fcntl(client->fd, F_GETFL) & O_ACCMODE) == O_RDONLY) {
scsi_build_sense(resp->sense, SENSE_CODE(INVALID_OPCODE));
sz = 0;
} else if (sz < PR_OUT_FIXED_PARAM_SIZE) {
/* Illegal request, Parameter list length error. This isn't fatal;
* we have read the data, send an error without closing the socket.
*/
scsi_build_sense(resp->sense, SENSE_CODE(INVALID_PARAM_LEN));
sz = 0;
}
if (sz == 0) {
resp->result = CHECK_CONDITION;
close(client->fd);
client->fd = -1;
}
} }
req->fd = client->fd; req->fd = client->fd;
@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque)
break; break;
} }
if (sz > 0) { num_active_sockets++;
num_active_sockets++; if (req.cdb[0] == PERSISTENT_RESERVE_OUT) {
if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { r = do_pr_out(req.fd, req.cdb, resp.sense,
r = do_pr_out(req.fd, req.cdb, resp.sense, client->data, sz);
client->data, sz); resp.sz = 0;
resp.sz = 0; } else {
} else { resp.sz = sizeof(client->data);
resp.sz = sizeof(client->data); r = do_pr_in(req.fd, req.cdb, resp.sense,
r = do_pr_in(req.fd, req.cdb, resp.sense, client->data, &resp.sz);
client->data, &resp.sz); resp.sz = MIN(resp.sz, sz);
resp.sz = MIN(resp.sz, sz);
}
num_active_sockets--;
close(req.fd);
if (r == -1) {
break;
}
resp.result = r;
} }
num_active_sockets--;
close(req.fd);
if (r == -1) {
break;
}
resp.result = r;
if (prh_write_response(client, &req, &resp, &local_err) < 0) { if (prh_write_response(client, &req, &resp, &local_err) < 0) {
break; break;