The prh_co_entry() routine handles requests. The first part is to read a
request by calling the prh_read_request() routine, if:
1. scsi_cdb_xfer(req->cdb) call returns 0, and
2. req->cdb[0] == PERSISTENT_RESERVE_IN, then
The resp->result field will be uninitialized. As a result the resp.sz
field will be also uninitialized in the prh_co_entry() function.
The second part is to send the response by calling the
prh_write_response() routine:
1. For the PERSISTENT_RESERVE_IN command, and
2. resp->result == GOOD (previous successful reply or just luck), then
There is a probability that the following assert will not be trigered:
assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data));
As a result some uninitialized response will be sent.
The fix is to initialize the response structure to CHECK_CONDITION and 0
values before calling the prh_read_request() routine.
Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
---
scsi/qemu-pr-helper.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index d0f8317..85878c2 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -768,6 +768,8 @@ static void coroutine_fn prh_co_entry(void *opaque)
PRHelperResponse resp;
int sz;
+ resp.result = CHECK_CONDITION;
+ resp.sz = 0;
sz = prh_read_request(client, &req, &resp, &local_err);
if (sz < 0) {
break;
--
2.7.4
Ping. On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote: > The prh_co_entry() routine handles requests. The first part is to read a > request by calling the prh_read_request() routine, if: > 1. scsi_cdb_xfer(req->cdb) call returns 0, and > 2. req->cdb[0] == PERSISTENT_RESERVE_IN, then > The resp->result field will be uninitialized. As a result the resp.sz > field will be also uninitialized in the prh_co_entry() function. > The second part is to send the response by calling the > prh_write_response() routine: > 1. For the PERSISTENT_RESERVE_IN command, and > 2. resp->result == GOOD (previous successful reply or just luck), then > There is a probability that the following assert will not be trigered: > assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data)); > As a result some uninitialized response will be sent. > > The fix is to initialize the response structure to CHECK_CONDITION and 0 > values before calling the prh_read_request() routine. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > --- > scsi/qemu-pr-helper.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index d0f8317..85878c2 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -768,6 +768,8 @@ static void coroutine_fn prh_co_entry(void *opaque) > PRHelperResponse resp; > int sz; > > + resp.result = CHECK_CONDITION; > + resp.sz = 0; > sz = prh_read_request(client, &req, &resp, &local_err); > if (sz < 0) { > break; > -- > 2.7.4 > >
On 02/07/2018 10:52, Dima Stepanov wrote: > Ping. > > On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote: >> The prh_co_entry() routine handles requests. The first part is to read a >> request by calling the prh_read_request() routine, if: >> 1. scsi_cdb_xfer(req->cdb) call returns 0, and >> 2. req->cdb[0] == PERSISTENT_RESERVE_IN, then >> The resp->result field will be uninitialized. As a result the resp.sz >> field will be also uninitialized in the prh_co_entry() function. >> The second part is to send the response by calling the >> prh_write_response() routine: >> 1. For the PERSISTENT_RESERVE_IN command, and >> 2. resp->result == GOOD (previous successful reply or just luck), then >> There is a probability that the following assert will not be trigered: >> assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data)); >> As a result some uninitialized response will be sent. >> >> The fix is to initialize the response structure to CHECK_CONDITION and 0 >> values before calling the prh_read_request() routine. The actual bug is that the "if (sz > 0)" should apply only to PERSISTENT_RESERVE_OUT, and in fact it can be done in do_pr_out. PERSISTENT_RESERVE_IN with sz == 0 is weird but okay. This simplifies the code a bit too, because we can handle closing the file descriptor in prh_co_entry. Does something like this work for you? diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c index 0218d65bbf..c89a446a45 100644 --- a/scsi/qemu-pr-helper.c +++ b/scsi/qemu-pr-helper.c @@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, char transportids[PR_HELPER_DATA_SIZE]; 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) { case MPATH_PROUT_REG_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) { 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 if (is_mpath(fd)) { return multipath_pr_out(fd, cdb, sense, param, sz); @@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient *client, errp) < 0) { 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; @@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque) break; } - if (sz > 0) { - num_active_sockets++; - if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { - r = do_pr_out(req.fd, req.cdb, resp.sense, - client->data, sz); - resp.sz = 0; - } else { - resp.sz = sizeof(client->data); - r = do_pr_in(req.fd, req.cdb, resp.sense, - client->data, &resp.sz); - resp.sz = MIN(resp.sz, sz); - } - num_active_sockets--; - close(req.fd); - if (r == -1) { - break; - } - resp.result = r; + num_active_sockets++; + if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { + r = do_pr_out(req.fd, req.cdb, resp.sense, + client->data, sz); + resp.sz = 0; + } else { + resp.sz = sizeof(client->data); + r = do_pr_in(req.fd, req.cdb, resp.sense, + client->data, &resp.sz); + resp.sz = MIN(resp.sz, sz); + } + num_active_sockets--; + close(req.fd); + if (r == -1) { + break; } + resp.result = r; if (prh_write_response(client, &req, &resp, &local_err) < 0) { break;
On Mon, Jul 02, 2018 at 02:21:41PM +0200, Paolo Bonzini wrote: > On 02/07/2018 10:52, Dima Stepanov wrote: > > Ping. > > > > On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote: > >> The prh_co_entry() routine handles requests. The first part is to read a > >> request by calling the prh_read_request() routine, if: > >> 1. scsi_cdb_xfer(req->cdb) call returns 0, and > >> 2. req->cdb[0] == PERSISTENT_RESERVE_IN, then > >> The resp->result field will be uninitialized. As a result the resp.sz > >> field will be also uninitialized in the prh_co_entry() function. > >> The second part is to send the response by calling the > >> prh_write_response() routine: > >> 1. For the PERSISTENT_RESERVE_IN command, and > >> 2. resp->result == GOOD (previous successful reply or just luck), then > >> There is a probability that the following assert will not be trigered: > >> assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data)); > >> As a result some uninitialized response will be sent. > >> > >> The fix is to initialize the response structure to CHECK_CONDITION and 0 > >> values before calling the prh_read_request() routine. > > The actual bug is that the "if (sz > 0)" should apply only to > PERSISTENT_RESERVE_OUT, and in fact it can be done in do_pr_out. > PERSISTENT_RESERVE_IN with sz == 0 is weird but okay. > > This simplifies the code a bit too, because we can handle closing the > file descriptor in prh_co_entry. > > Does something like this work for you? Thanks for the feedback. Yes, this will work for me. Should i update the patch and resend it or you will just pick the version you suggested? Regards, Dima. > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index 0218d65bbf..c89a446a45 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -455,6 +455,14 @@ static int multipath_pr_out(int fd, const uint8_t *cdb, uint8_t *sense, > char transportids[PR_HELPER_DATA_SIZE]; > 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) { > case MPATH_PROUT_REG_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) > { > 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 > if (is_mpath(fd)) { > return multipath_pr_out(fd, cdb, sense, param, sz); > @@ -690,21 +704,6 @@ static int coroutine_fn prh_read_request(PRHelperClient *client, > errp) < 0) { > 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; > @@ -785,25 +784,23 @@ static void coroutine_fn prh_co_entry(void *opaque) > break; > } > > - if (sz > 0) { > - num_active_sockets++; > - if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { > - r = do_pr_out(req.fd, req.cdb, resp.sense, > - client->data, sz); > - resp.sz = 0; > - } else { > - resp.sz = sizeof(client->data); > - r = do_pr_in(req.fd, req.cdb, resp.sense, > - client->data, &resp.sz); > - resp.sz = MIN(resp.sz, sz); > - } > - num_active_sockets--; > - close(req.fd); > - if (r == -1) { > - break; > - } > - resp.result = r; > + num_active_sockets++; > + if (req.cdb[0] == PERSISTENT_RESERVE_OUT) { > + r = do_pr_out(req.fd, req.cdb, resp.sense, > + client->data, sz); > + resp.sz = 0; > + } else { > + resp.sz = sizeof(client->data); > + r = do_pr_in(req.fd, req.cdb, resp.sense, > + client->data, &resp.sz); > + resp.sz = MIN(resp.sz, sz); > + } > + num_active_sockets--; > + close(req.fd); > + if (r == -1) { > + break; > } > + resp.result = r; > > if (prh_write_response(client, &req, &resp, &local_err) < 0) { > break;
On 03/07/2018 11:27, Dima Stepanov wrote: > On Mon, Jul 02, 2018 at 02:21:41PM +0200, Paolo Bonzini wrote: >> On 02/07/2018 10:52, Dima Stepanov wrote: >>> Ping. >>> >>> On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote: >>>> The prh_co_entry() routine handles requests. The first part is to read a >>>> request by calling the prh_read_request() routine, if: >>>> 1. scsi_cdb_xfer(req->cdb) call returns 0, and >>>> 2. req->cdb[0] == PERSISTENT_RESERVE_IN, then >>>> The resp->result field will be uninitialized. As a result the resp.sz >>>> field will be also uninitialized in the prh_co_entry() function. >>>> The second part is to send the response by calling the >>>> prh_write_response() routine: >>>> 1. For the PERSISTENT_RESERVE_IN command, and >>>> 2. resp->result == GOOD (previous successful reply or just luck), then >>>> There is a probability that the following assert will not be trigered: >>>> assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data)); >>>> As a result some uninitialized response will be sent. >>>> >>>> The fix is to initialize the response structure to CHECK_CONDITION and 0 >>>> values before calling the prh_read_request() routine. >> >> The actual bug is that the "if (sz > 0)" should apply only to >> PERSISTENT_RESERVE_OUT, and in fact it can be done in do_pr_out. >> PERSISTENT_RESERVE_IN with sz == 0 is weird but okay. >> >> This simplifies the code a bit too, because we can handle closing the >> file descriptor in prh_co_entry. >> >> Does something like this work for you? > > Thanks for the feedback. Yes, this will work for me. Should i update the > patch and resend it or you will just pick the version you suggested? I will pick it, thanks! Paolo
On Fri, Jun 15, 2018 at 12:11:44PM +0300, Dima Stepanov wrote: > The prh_co_entry() routine handles requests. The first part is to read a > request by calling the prh_read_request() routine, if: > 1. scsi_cdb_xfer(req->cdb) call returns 0, and > 2. req->cdb[0] == PERSISTENT_RESERVE_IN, then > The resp->result field will be uninitialized. As a result the resp.sz > field will be also uninitialized in the prh_co_entry() function. > The second part is to send the response by calling the > prh_write_response() routine: > 1. For the PERSISTENT_RESERVE_IN command, and > 2. resp->result == GOOD (previous successful reply or just luck), then > There is a probability that the following assert will not be trigered: > assert(resp->sz <= req->sz && resp->sz <= sizeof(client->data)); > As a result some uninitialized response will be sent. > > The fix is to initialize the response structure to CHECK_CONDITION and 0 > values before calling the prh_read_request() routine. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > --- > scsi/qemu-pr-helper.c | 2 ++ > 1 file changed, 2 insertions(+) CCing Paolo Bonzini, SCSI maintainer. You can use scripts/get_maintainer.pl -f scsi/qemu-pr-helper.c to find relevant people to CC on a patch. > > diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c > index d0f8317..85878c2 100644 > --- a/scsi/qemu-pr-helper.c > +++ b/scsi/qemu-pr-helper.c > @@ -768,6 +768,8 @@ static void coroutine_fn prh_co_entry(void *opaque) > PRHelperResponse resp; > int sz; > > + resp.result = CHECK_CONDITION; > + resp.sz = 0; > sz = prh_read_request(client, &req, &resp, &local_err); > if (sz < 0) { > break; > -- > 2.7.4 > >
© 2016 - 2024 Red Hat, Inc.