| Message ID | 151520104838.32271.7038801336240727574.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Jan 05, 2018 at 05:10:48PM -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. 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. I'm pretty sure that 'handle' comes from the hardware, not from userspace, from what I can tell here. If we want to start auditing __iomem data sources, great! But that's a bigger task, and one I don't think we are ready to tackle... thanks, greg k-h
On Sat, Jan 06, 2018 at 10:03:22AM +0100, Greg KH wrote: > On Fri, Jan 05, 2018 at 05:10:48PM -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. 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. > > I'm pretty sure that 'handle' comes from the hardware, not from > userspace, from what I can tell here. If we want to start auditing > __iomem data sources, great! But that's a bigger task, and one I don't > think we are ready to tackle... And as Peter Zijlstra has already mentioned, if we have to look at those codepaths, USB drivers are the first up for that mess, so having access to the coverity rules would be a great help in starting that effort. thanks, greg k-h
On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > On Fri, Jan 05, 2018 at 05:10:48PM -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. 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. > > I'm pretty sure that 'handle' comes from the hardware, not from > userspace, from what I can tell here. If we want to start auditing > __iomem data sources, great! But that's a bigger task, and one I don't > think we are ready to tackle... I think it falls in the hygiene bucket of shutting off an array index from a source that could be under attacker control. Should we leave this one un-patched while we decide if we generally have a problem with trusting completion 'tags' from hardware? My vote is patch it for now.
On Thu, Jan 11, 2018 at 02:15:12PM -0800, Dan Williams wrote: > On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Jan 05, 2018 at 05:10:48PM -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. 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. > > > > I'm pretty sure that 'handle' comes from the hardware, not from > > userspace, from what I can tell here. If we want to start auditing > > __iomem data sources, great! But that's a bigger task, and one I don't > > think we are ready to tackle... > > I think it falls in the hygiene bucket of shutting off an array index > from a source that could be under attacker control. Should we leave > this one un-patched while we decide if we generally have a problem > with trusting completion 'tags' from hardware? My vote is patch it for > now. Hah, if you are worried about "tags" from hardware, we have a lot more auditing to do, right? I don't think anyone has looked into just basic "bounds checking" for that type of information. For USB devices we have _just_ started doing that over the past year, the odds of anyone looking at PCI devices for this same problem is slim-to-none. Again, here are my questions/objections right now to this series: - How can we audit this stuff? - How did you audit this stuff to find these usages? - How do you know that this series fixes all of the issues? - What exact tree/date did you run your audit against? - How do you know that linux-next does not contain a boatload more problems that we need to go back and fix after 4.16-rc1 is out? - How can we prevent this type of pattern showing up again? - How can we audit the data coming from hardware correctly? I'm all for merging this series, but if anyone things that somehow the whole problem is now "solved" in this area, they are sorely mistaken. thanks, greg k-h
On Fri, 2018-01-12 at 08:27 +0100, Greg KH wrote: > On Thu, Jan 11, 2018 at 02:15:12PM -0800, Dan Williams wrote: > > > > On Sat, Jan 6, 2018 at 1:03 AM, Greg KH <gregkh@linuxfoundation.org > > > wrote: > > > > > > On Fri, Jan 05, 2018 at 05:10:48PM -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. 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. > > > > > > I'm pretty sure that 'handle' comes from the hardware, not from > > > userspace, from what I can tell here. If we want to start > > > auditing __iomem data sources, great! But that's a bigger task, > > > and one I don't think we are ready to tackle... > > > > I think it falls in the hygiene bucket of shutting off an array > > index from a source that could be under attacker control. Should we > > leave this one un-patched while we decide if we generally have a > > problem with trusting completion 'tags' from hardware? My vote is > > patch it for now. > > Hah, if you are worried about "tags" from hardware, we have a lot > more auditing to do, right? We'd also have a lot more to do: the assumption would have to be malicious hardware and most hardware has access to fairly vital stuff directly. I really don't think we have to worry about side channel attacks from hardware until the direct attack vector is closed. James
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c index d5da3981cefe..128b41de3784 100644 --- a/drivers/scsi/qla2xxx/qla_mr.c +++ b/drivers/scsi/qla2xxx/qla_mr.c @@ -9,6 +9,7 @@ #include <linux/ktime.h> #include <linux/pci.h> #include <linux/ratelimit.h> +#include <linux/compiler.h> #include <linux/vmalloc.h> #include <linux/bsg-lib.h> #include <scsi/scsi_tcq.h> @@ -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,9 @@ 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]; + if ((elem = nospec_array_ptr(req->outstanding_cmds, handle, + req->num_outstanding_cmds))) + sp = *elem; else sp = NULL; @@ -2626,7 +2628,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 +2657,9 @@ 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]; + if ((elem = nospec_array_ptr(req->outstanding_cmds, handle, + req->num_outstanding_cmds))) + sp = *elem; else sp = NULL;