* [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data @ 2018-06-15 9:11 Dima Stepanov 2018-07-02 8:52 ` Dima Stepanov 2018-07-02 10:38 ` Stefan Hajnoczi 0 siblings, 2 replies; 6+ messages in thread From: Dima Stepanov @ 2018-06-15 9:11 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data 2018-06-15 9:11 [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data Dima Stepanov @ 2018-07-02 8:52 ` Dima Stepanov 2018-07-02 12:21 ` Paolo Bonzini 2018-07-02 10:38 ` Stefan Hajnoczi 1 sibling, 1 reply; 6+ messages in thread From: Dima Stepanov @ 2018-07-02 8:52 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh, pbonzini 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data 2018-07-02 8:52 ` Dima Stepanov @ 2018-07-02 12:21 ` Paolo Bonzini 2018-07-03 9:27 ` Dima Stepanov 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2018-07-02 12:21 UTC (permalink / raw) To: Dima Stepanov, qemu-devel; +Cc: wrfsh 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; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data 2018-07-02 12:21 ` Paolo Bonzini @ 2018-07-03 9:27 ` Dima Stepanov 2018-07-03 9:35 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Dima Stepanov @ 2018-07-03 9:27 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, wrfsh 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; ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data 2018-07-03 9:27 ` Dima Stepanov @ 2018-07-03 9:35 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2018-07-03 9:35 UTC (permalink / raw) To: Dima Stepanov; +Cc: qemu-devel, wrfsh 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data 2018-06-15 9:11 [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data Dima Stepanov 2018-07-02 8:52 ` Dima Stepanov @ 2018-07-02 10:38 ` Stefan Hajnoczi 1 sibling, 0 replies; 6+ messages in thread From: Stefan Hajnoczi @ 2018-07-02 10:38 UTC (permalink / raw) To: Dima Stepanov; +Cc: qemu-devel, wrfsh, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 1793 bytes --] 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 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 455 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-07-03 9:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-15 9:11 [Qemu-devel] [PATCH v1] qemu-pr-helper: garbage response structure can be used to write data Dima Stepanov 2018-07-02 8:52 ` Dima Stepanov 2018-07-02 12:21 ` Paolo Bonzini 2018-07-03 9:27 ` Dima Stepanov 2018-07-03 9:35 ` Paolo Bonzini 2018-07-02 10:38 ` Stefan Hajnoczi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.