From: "Roger Pau Monné" <roger.pau@citrix.com> To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com> Cc: <xen-devel@lists.xenproject.org>, <stable@vger.kernel.org>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Juergen Gross <jgross@suse.com>, Jens Axboe <axboe@kernel.dk>, "open list:BLOCK LAYER" <linux-block@vger.kernel.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 6/6] xen-blkfront: prepare request locally, only then put it on the shared ring Date: Tue, 1 May 2018 09:22:31 +0100 [thread overview] Message-ID: <20180501082231.dzdbcghtwvlbkoys@MacBook-Pro-de-Roger.local> (raw) In-Reply-To: <951a221b0e655b3077d1f96ac365194320bc8809.1525122026.git-series.marmarek@invisiblethingslab.com> On Mon, Apr 30, 2018 at 11:01:50PM +0200, Marek Marczykowski-G�recki wrote: > Do not reuse data which theoretically might be already modified by the > backend. This is mostly about private copy of the request > (info->shadow[id].req) - make sure the request saved there is really the > one just filled. > > This is complementary to XSA155. > > CC: stable@vger.kernel.org > Signed-off-by: Marek Marczykowski-G�recki <marmarek@invisiblethingslab.com> > --- > drivers/block/xen-blkfront.c | 76 +++++++++++++++++++++---------------- > 1 file changed, 44 insertions(+), 32 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 3926811..b100b55 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -525,19 +525,16 @@ static int blkif_ioctl(struct block_device *bdev, fmode_t mode, > > static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, The name of this function should be changed IMO, since you are no longer getting a request from the ring, but just initializing a request struct. > struct request *req, > - struct blkif_request **ring_req) > + struct blkif_request *ring_req) > { > unsigned long id; > > - *ring_req = RING_GET_REQUEST(&rinfo->ring, rinfo->ring.req_prod_pvt); > - rinfo->ring.req_prod_pvt++; > - > id = get_id_from_freelist(rinfo); > rinfo->shadow[id].request = req; > rinfo->shadow[id].status = REQ_WAITING; > rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID; > > - (*ring_req)->u.rw.id = id; > + ring_req->u.rw.id = id; > > return id; > } > @@ -545,23 +542,28 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, > static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_info *rinfo) > { > struct blkfront_info *info = rinfo->dev_info; > - struct blkif_request *ring_req; > + struct blkif_request ring_req = { 0 }; > unsigned long id; > > /* Fill out a communications ring structure. */ > id = blkif_ring_get_request(rinfo, req, &ring_req); Maybe I'm missing something obvious here, but you are adding a struct allocated on the stack to the shadow ring copy, isn't this dangerous? The pointer stored in the shadow ring copy is going to be invalid after returning from this function. The same comment applies to the other calls to blkif_ring_get_request below that pass a ring_reg allocated on the stack. Thanks, Roger.
WARNING: multiple messages have this Message-ID (diff)
From: "Roger Pau Monné" <roger.pau@citrix.com> To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com> Cc: <xen-devel@lists.xenproject.org>, <stable@vger.kernel.org>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, Boris Ostrovsky <boris.ostrovsky@oracle.com>, Juergen Gross <jgross@suse.com>, Jens Axboe <axboe@kernel.dk>, "open list:BLOCK LAYER" <linux-block@vger.kernel.org>, open list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 6/6] xen-blkfront: prepare request locally, only then put it on the shared ring Date: Tue, 1 May 2018 09:22:31 +0100 [thread overview] Message-ID: <20180501082231.dzdbcghtwvlbkoys@MacBook-Pro-de-Roger.local> (raw) In-Reply-To: <951a221b0e655b3077d1f96ac365194320bc8809.1525122026.git-series.marmarek@invisiblethingslab.com> On Mon, Apr 30, 2018 at 11:01:50PM +0200, Marek Marczykowski-Górecki wrote: > Do not reuse data which theoretically might be already modified by the > backend. This is mostly about private copy of the request > (info->shadow[id].req) - make sure the request saved there is really the > one just filled. > > This is complementary to XSA155. > > CC: stable@vger.kernel.org > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > drivers/block/xen-blkfront.c | 76 +++++++++++++++++++++---------------- > 1 file changed, 44 insertions(+), 32 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index 3926811..b100b55 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -525,19 +525,16 @@ static int blkif_ioctl(struct block_device *bdev, fmode_t mode, > > static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, The name of this function should be changed IMO, since you are no longer getting a request from the ring, but just initializing a request struct. > struct request *req, > - struct blkif_request **ring_req) > + struct blkif_request *ring_req) > { > unsigned long id; > > - *ring_req = RING_GET_REQUEST(&rinfo->ring, rinfo->ring.req_prod_pvt); > - rinfo->ring.req_prod_pvt++; > - > id = get_id_from_freelist(rinfo); > rinfo->shadow[id].request = req; > rinfo->shadow[id].status = REQ_WAITING; > rinfo->shadow[id].associated_id = NO_ASSOCIATED_ID; > > - (*ring_req)->u.rw.id = id; > + ring_req->u.rw.id = id; > > return id; > } > @@ -545,23 +542,28 @@ static unsigned long blkif_ring_get_request(struct blkfront_ring_info *rinfo, > static int blkif_queue_discard_req(struct request *req, struct blkfront_ring_info *rinfo) > { > struct blkfront_info *info = rinfo->dev_info; > - struct blkif_request *ring_req; > + struct blkif_request ring_req = { 0 }; > unsigned long id; > > /* Fill out a communications ring structure. */ > id = blkif_ring_get_request(rinfo, req, &ring_req); Maybe I'm missing something obvious here, but you are adding a struct allocated on the stack to the shadow ring copy, isn't this dangerous? The pointer stored in the shadow ring copy is going to be invalid after returning from this function. The same comment applies to the other calls to blkif_ring_get_request below that pass a ring_reg allocated on the stack. Thanks, Roger.
next prev parent reply other threads:[~2018-05-01 8:22 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-30 21:01 [PATCH 0/6] Fix XSA-155-like bugs in frontend drivers Marek Marczykowski-Górecki 2018-04-30 21:01 ` [PATCH 1/6] xen: Add RING_COPY_RESPONSE() Marek Marczykowski-Górecki 2018-04-30 21:25 ` Boris Ostrovsky 2018-04-30 21:25 ` Boris Ostrovsky 2018-04-30 21:27 ` Marek Marczykowski-Górecki 2018-04-30 21:41 ` Boris Ostrovsky 2018-04-30 21:41 ` Boris Ostrovsky 2018-04-30 21:27 ` Marek Marczykowski-Górecki 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-04-30 21:01 ` [PATCH 2/6] xen-netfront: copy response out of shared buffer before accessing it Marek Marczykowski-Górecki 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-05-02 5:20 ` Oleksandr Andrushchenko 2018-05-02 5:20 ` [Xen-devel] " Oleksandr Andrushchenko 2018-04-30 21:01 ` [PATCH 3/6] xen-netfront: do not use data already exposed to backend Marek Marczykowski-Górecki 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-04-30 21:01 ` [PATCH 4/6] xen-netfront: add range check for Tx response id Marek Marczykowski-Górecki 2018-05-01 10:05 ` Wei Liu 2018-05-01 10:05 ` [Xen-devel] " Wei Liu 2018-05-01 10:05 ` Wei Liu 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-04-30 21:01 ` [PATCH 5/6] xen-blkfront: make local copy of response before using it Marek Marczykowski-Górecki 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-04-30 21:01 ` [PATCH 6/6] xen-blkfront: prepare request locally, only then put it on the shared ring Marek Marczykowski-Górecki 2018-04-30 21:01 ` Marek Marczykowski-Górecki 2018-05-01 8:22 ` Roger Pau Monné 2018-05-01 8:22 ` Roger Pau Monné [this message] 2018-05-01 8:22 ` Roger Pau Monné 2018-05-01 9:15 ` Roger Pau Monné 2018-05-01 9:15 ` [Xen-devel] " Roger Pau Monné 2018-05-01 9:15 ` Roger Pau Monné 2018-05-01 10:12 ` [PATCH 0/6] Fix XSA-155-like bugs in frontend drivers Wei Liu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180501082231.dzdbcghtwvlbkoys@MacBook-Pro-de-Roger.local \ --to=roger.pau@citrix.com \ --cc=axboe@kernel.dk \ --cc=boris.ostrovsky@oracle.com \ --cc=jgross@suse.com \ --cc=konrad.wilk@oracle.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marmarek@invisiblethingslab.com \ --cc=stable@vger.kernel.org \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.