* [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-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
* 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
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.