Re: [PATCH v2 17/19] qla2xxx: prevent bounds-check bypass via speculative execution
On Thu, 2018-01-11 at 21:38 -0800, Dan Williams wrote: > On Thu, Jan 11, 2018 at 5:19 PM, James Bottomley > wrote: > > > > On Thu, 2018-01-11 at 16:47 -0800, Dan Williams wrote: > > > > > > Static analysis reports that 'handle' may be a user controlled > > > value that is used as a data dependency to read 'sp' from the > > > 'req->outstanding_cmds' array. > > > > Greg already told you it comes from hardware, specifically the > > hardware response queue. If you don't believe him, I can confirm > > it's quite definitely all copied from the iomem where the mailbox > > response is, so it can't be a user controlled value (well, unless > > the user has some influence over the firmware of the > > qla2xxx controller, which probably means you have other things to > > worry about than speculative information leaks). > > I do believe him, and I still submitted this. I'm trying to probe at > the meta question of where do we draw the line with these especially > when it costs us relatively little to apply a few line patch? We fix > theoretical lockdep races, why not theoretical data leak paths? I think I've lost the thread of what you're after. I thought you were asking for the domain experts to look and see if there is the potential for attack; if there's no theoretical way for a user to influence the value what's the point of killing speculation? Furthermore, if the user could affect that 32 bit value, what they'd actually do is extract information via the que variable which you didn't fix and which could be used to compromise the kernel without resorting to side channel attacks. What's most puzzling to me is the inconsistency of the positions: if it doesn't cost that much to turn off speculation, just do it on kernel entry as Jiří suggested; we can make it a dynamic option and the cloud providers can do it and the rest of us don't need to bother. If it does cost a lot to turn it off as Alan said, then you need us to identify the cases above where there's no need to disrupt the speculation pipeline and not turn it off there. Which is it? James
Re: [PATCH v2 17/19] qla2xxx: prevent bounds-check bypass via speculative execution
On Thu, Jan 11, 2018 at 5:19 PM, James Bottomley wrote: > On Thu, 2018-01-11 at 16:47 -0800, Dan Williams wrote: >> Static analysis reports that 'handle' may be a user controlled value >> that is used as a data dependency to read 'sp' from the >> 'req->outstanding_cmds' array. > > Greg already told you it comes from hardware, specifically the hardware > response queue. If you don't believe him, I can confirm it's quite > definitely all copied from the iomem where the mailbox response is, so > it can't be a user controlled value (well, unless the user has some > influence over the firmware of the qla2xxx controller, which probably > means you have other things to worry about than speculative information > leaks). I do believe him, and I still submitted this. I'm trying to probe at the meta question of where do we draw the line with these especially when it costs us relatively little to apply a few line patch? We fix theoretical lockdep races, why not theoretical data leak paths?
Re: [PATCH v2 17/19] qla2xxx: prevent bounds-check bypass via speculative execution
On Thu, 2018-01-11 at 16:47 -0800, Dan Williams wrote: > Static analysis reports that 'handle' may be a user controlled value > that is used as a data dependency to read 'sp' from the > 'req->outstanding_cmds' array. Greg already told you it comes from hardware, specifically the hardware response queue. If you don't believe him, I can confirm it's quite definitely all copied from the iomem where the mailbox response is, so it can't be a user controlled value (well, unless the user has some influence over the firmware of the qla2xxx controller, which probably means you have other things to worry about than speculative information leaks). I think what it actually is is a handle returned in the mailbox that's used to find other mailbox entries in different queues (the packet handle actually contains an entry in the lower 16 bits and a queue designation in the upper). Perhaps the qla2xxx people should comment on this because the code seems to check and print an error if there's a problem with the handle being too big, but we don't check the que value and use it blindly to read into the req_q_map: if handle could be wrong, couldn't que? James
[PATCH v2 17/19] qla2xxx: prevent bounds-check bypass via speculative execution
Static analysis reports that 'handle' may be a user controlled value
that is used as a data dependency to read 'sp' from the
'req->outstanding_cmds' array. In order to avoid potential leaks of
kernel memory values, block speculative execution of the instruction
stream that could issue reads based on an invalid value of 'sp'. In this
case 'sp' is directly dereferenced later in the function.
If completion tags can be specified by userspace or otherwise
controlled, we should sanitize 'handle' with array_ptr.
Based on an original patch by Elena Reshetova.
Cc: qla2xxx-upstr...@qlogic.com
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Elena Reshetova
Signed-off-by: Dan Williams
---
drivers/scsi/qla2xxx/qla_mr.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index d5da3981cefe..6de3f0fddcfc 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -2275,7 +2276,7 @@ qlafx00_ioctl_iosb_entry(scsi_qla_host_t *vha, struct
req_que *req,
static void
qlafx00_status_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, void *pkt)
{
- srb_t *sp;
+ srb_t *sp, **elem;
fc_port_t *fcport;
struct scsi_cmnd *cp;
struct sts_entry_fx00 *sts;
@@ -2304,8 +2305,10 @@ qlafx00_status_entry(scsi_qla_host_t *vha, struct
rsp_que *rsp, void *pkt)
req = ha->req_q_map[que];
/* Validate handle. */
- if (handle < req->num_outstanding_cmds)
- sp = req->outstanding_cmds[handle];
+ elem = array_ptr(req->outstanding_cmds, handle,
+ req->num_outstanding_cmds);
+ if (elem)
+ sp = *elem;
else
sp = NULL;
@@ -2626,7 +2629,7 @@ static void
qlafx00_multistatus_entry(struct scsi_qla_host *vha,
struct rsp_que *rsp, void *pkt)
{
- srb_t *sp;
+ srb_t *sp, **elem;
struct multi_sts_entry_fx00 *stsmfx;
struct qla_hw_data *ha = vha->hw;
uint32_t handle, hindex, handle_count, i;
@@ -2655,8 +2658,10 @@ qlafx00_multistatus_entry(struct scsi_qla_host *vha,
req = ha->req_q_map[que];
/* Validate handle. */
- if (handle < req->num_outstanding_cmds)
- sp = req->outstanding_cmds[handle];
+ elem = array_ptr(req->outstanding_cmds, handle,
+ req->num_outstanding_cmds);
+ if (elem)
+ sp = *elem;
else
sp = NULL;
