[PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Juergen Gross]

A request in the ring buffer mustn't be read after it has been marked
as consumed. Otherwise it might already have been reused by the
frontend without violating the ring protocol.

To avoid inconsistencies in the backend only work on a private copy
of the request. This will ensure a malicious guest not being able to
bypass consistency checks of the backend by modifying an active
request.

Signed-off-by: Juergen Gross <jgross@suse.com>

diff -r 578e5aea3cbb drivers/xen/scsiback/scsiback.c
--- a/drivers/xen/scsiback/scsiback.c	Mon Jan 19 11:51:46 2015 +0100
+++ b/drivers/xen/scsiback/scsiback.c	Fri Jan 30 14:43:29 2015 +0100
@@ -579,7 +579,7 @@ invalid_value:
 static int _scsiback_do_cmd_fn(struct vscsibk_info *info)
 {
 	struct vscsiif_back_ring *ring = &info->ring;
-	vscsiif_request_t  *ring_req;
+	vscsiif_request_t ring_req;
 
 	pending_req_t *pending_req;
 	RING_IDX rc, rp;
@@ -609,10 +609,10 @@ static int _scsiback_do_cmd_fn(struct vs
 			break;
 		}
 
-		ring_req = RING_GET_REQUEST(ring, rc);
+		ring_req = *RING_GET_REQUEST(ring, rc);
 		ring->req_cons = ++rc;
 
-		err = prepare_pending_reqs(info, ring_req,
+		err = prepare_pending_reqs(info, &ring_req,
 						pending_req);
 		switch (err ?: pending_req->act) {
 		case VSCSIIF_ACT_SCSI_CDB:

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Jan Beulich]

>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
> A request in the ring buffer mustn't be read after it has been marked
> as consumed. Otherwise it might already have been reused by the
> frontend without violating the ring protocol.
> 
> To avoid inconsistencies in the backend only work on a private copy
> of the request. This will ensure a malicious guest not being able to
> bypass consistency checks of the backend by modifying an active
> request.

I'm not convinced we need this in this version of the driver: c/s
590:c4134d1a3e3f took care of reading each ring_req field just
once.

Jan

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Juergen Gross]

On 01/30/2015 03:22 PM, Jan Beulich wrote:
>>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
>> A request in the ring buffer mustn't be read after it has been marked
>> as consumed. Otherwise it might already have been reused by the
>> frontend without violating the ring protocol.
>>
>> To avoid inconsistencies in the backend only work on a private copy
>> of the request. This will ensure a malicious guest not being able to
>> bypass consistency checks of the backend by modifying an active
>> request.
>
> I'm not convinced we need this in this version of the driver: c/s
> 590:c4134d1a3e3f took care of reading each ring_req field just
> once.

This might be true. But the consumer index is incremented before the
last item of the request is read. This is a violation of the ring
interface: the frontend is free to put another request in this slot
while the backend is still using it.


Juergen

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Jan Beulich]

>>> On 30.01.15 at 15:22, <JBeulich@suse.com> wrote:
>>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
>> A request in the ring buffer mustn't be read after it has been marked
>> as consumed. Otherwise it might already have been reused by the
>> frontend without violating the ring protocol.
>> 
>> To avoid inconsistencies in the backend only work on a private copy
>> of the request. This will ensure a malicious guest not being able to
>> bypass consistency checks of the backend by modifying an active
>> request.
> 
> I'm not convinced we need this in this version of the driver: c/s
> 590:c4134d1a3e3f took care of reading each ring_req field just
> once.

I should have clarified that I didn't mean we don't need to change
anything here: We should still move down the point where the
ring slot gets accounted as consumed.

Jan

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Juergen Gross]

On 01/30/2015 03:32 PM, Jan Beulich wrote:
>>>> On 30.01.15 at 15:22, <JBeulich@suse.com> wrote:
>>>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
>>> A request in the ring buffer mustn't be read after it has been marked
>>> as consumed. Otherwise it might already have been reused by the
>>> frontend without violating the ring protocol.
>>>
>>> To avoid inconsistencies in the backend only work on a private copy
>>> of the request. This will ensure a malicious guest not being able to
>>> bypass consistency checks of the backend by modifying an active
>>> request.
>>
>> I'm not convinced we need this in this version of the driver: c/s
>> 590:c4134d1a3e3f took care of reading each ring_req field just
>> once.
>
> I should have clarified that I didn't mean we don't need to change
> anything here: We should still move down the point where the
> ring slot gets accounted as consumed.

My solution is more robust, I think. You don't have to be careful not
to introduce another double read somewhere.

Juergen

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Jan Beulich]

>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
> A request in the ring buffer mustn't be read after it has been marked
> as consumed. Otherwise it might already have been reused by the
> frontend without violating the ring protocol.

This is irrelevant, as the ->req_cons is a backend private field (if it
was a shared one, a barrier would have been needed between
copying and updating that field).

> To avoid inconsistencies in the backend only work on a private copy
> of the request. This will ensure a malicious guest not being able to
> bypass consistency checks of the backend by modifying an active
> request.

Hence - also considering with my earlier reply - I don't view the change
as necessary.

Jan

Re: [PATCH linux-2.6.18] xen: mark pvscsi frontend request consumed only after last read

[Juergen Gross]

On 02/02/2015 08:52 AM, Jan Beulich wrote:
>>>> On 30.01.15 at 14:51, <"jgross@suse.com".non-mime.internet> wrote:
>> A request in the ring buffer mustn't be read after it has been marked
>> as consumed. Otherwise it might already have been reused by the
>> frontend without violating the ring protocol.
>
> This is irrelevant, as the ->req_cons is a backend private field (if it
> was a shared one, a barrier would have been needed between
> copying and updating that field).

Hmm, you are right. Interesting, I've always thought req_cons would be
used by the frontend, too.

Thanks for updating my protocol know-how. :-)

>
>> To avoid inconsistencies in the backend only work on a private copy
>> of the request. This will ensure a malicious guest not being able to
>> bypass consistency checks of the backend by modifying an active
>> request.
>
> Hence - also considering with my earlier reply - I don't view the change
> as necessary.

Agreed.


Juergen