* [PATCH] intel_th: Fix a missing-check bug @ 2018-10-19 13:46 Wenwen Wang 2018-10-29 18:39 ` Wenwen Wang 0 siblings, 1 reply; 3+ messages in thread From: Wenwen Wang @ 2018-10-19 13:46 UTC (permalink / raw) To: Wenwen Wang; +Cc: Kangjie Lu, Alexander Shishkin, open list In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc' is firstly checked to see whether the descriptor has a valid data width. If yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor will be returned. It is worth noting that the data size is calculated from 'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a consistent DMA region, which is allocated through dma_alloc_coherent() in msc_buffer_win_alloc(). Given that the device also has the permission to access this DMA region, it is possible that a malicious device controlled by an attacker can modify the 'valid_dw' field after the if statement but before the return statement in msc_data_sz(). Through this way, the device can bypass the check and supply unexpected data width. This patch copies the 'valid_dw' field to a local variable and then performs the check and the calculation on the local variable to avoid the above issue. Signed-off-by: Wenwen Wang <wang6495@umn.edu> --- drivers/hwtracing/intel_th/msu.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h index 9cc8ace..b7d846e 100644 --- a/drivers/hwtracing/intel_th/msu.h +++ b/drivers/hwtracing/intel_th/msu.h @@ -79,10 +79,12 @@ struct msc_block_desc { static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc) { - if (!bdesc->valid_dw) + u32 valid_dw = bdesc->valid_dw; + + if (!valid_dw) return 0; - return bdesc->valid_dw * 4 - MSC_BDESC; + return valid_dw * 4 - MSC_BDESC; } static inline bool msc_block_wrapped(struct msc_block_desc *bdesc) -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] intel_th: Fix a missing-check bug 2018-10-19 13:46 [PATCH] intel_th: Fix a missing-check bug Wenwen Wang @ 2018-10-29 18:39 ` Wenwen Wang 2018-10-30 11:15 ` Alexander Shishkin 0 siblings, 1 reply; 3+ messages in thread From: Wenwen Wang @ 2018-10-29 18:39 UTC (permalink / raw) To: Wenwen Wang; +Cc: Kangjie Lu, alexander.shishkin, open list Hello, Can anyone confirm this bug? Thanks! Wenwen On Fri, Oct 19, 2018 at 8:47 AM Wenwen Wang <wang6495@umn.edu> wrote: > > In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc' > is firstly checked to see whether the descriptor has a valid data width. If > yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor > will be returned. It is worth noting that the data size is calculated from > 'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a > consistent DMA region, which is allocated through dma_alloc_coherent() in > msc_buffer_win_alloc(). Given that the device also has the permission to > access this DMA region, it is possible that a malicious device controlled > by an attacker can modify the 'valid_dw' field after the if statement but > before the return statement in msc_data_sz(). Through this way, the device > can bypass the check and supply unexpected data width. > > This patch copies the 'valid_dw' field to a local variable and then > performs the check and the calculation on the local variable to avoid the > above issue. > > Signed-off-by: Wenwen Wang <wang6495@umn.edu> > --- > drivers/hwtracing/intel_th/msu.h | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h > index 9cc8ace..b7d846e 100644 > --- a/drivers/hwtracing/intel_th/msu.h > +++ b/drivers/hwtracing/intel_th/msu.h > @@ -79,10 +79,12 @@ struct msc_block_desc { > > static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc) > { > - if (!bdesc->valid_dw) > + u32 valid_dw = bdesc->valid_dw; > + > + if (!valid_dw) > return 0; > > - return bdesc->valid_dw * 4 - MSC_BDESC; > + return valid_dw * 4 - MSC_BDESC; > } > > static inline bool msc_block_wrapped(struct msc_block_desc *bdesc) > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] intel_th: Fix a missing-check bug 2018-10-29 18:39 ` Wenwen Wang @ 2018-10-30 11:15 ` Alexander Shishkin 0 siblings, 0 replies; 3+ messages in thread From: Alexander Shishkin @ 2018-10-30 11:15 UTC (permalink / raw) To: Wenwen Wang, Wenwen Wang; +Cc: Kangjie Lu, open list Wenwen Wang <wang6495@umn.edu> writes: > Hello, > > Can anyone confirm this bug? Thanks! Commonly this burden lies with the author of the patch. If you fix a bug, you need to be able to demonstrate it. If it's a mere hypothesis, there needs to be a detailed analysis of how exactly can this be exploited and what is the potential outcome of an exploit. Some other questions that arise from looking at the patch, for example, does gcc actually generate different code as a result of this patch? > On Fri, Oct 19, 2018 at 8:47 AM Wenwen Wang <wang6495@umn.edu> wrote: >> >> In msc_data_sz(), the 'valid_dw' field of the msc block descriptor 'bdesc' >> is firstly checked to see whether the descriptor has a valid data width. If >> yes, i.e., 'bdesc->valid_dw' is not 0, the data size of this descriptor >> will be returned. It is worth noting that the data size is calculated from >> 'bdesc->valid_dw'. The problem here is that 'bdesc' actually points to a >> consistent DMA region, which is allocated through dma_alloc_coherent() in >> msc_buffer_win_alloc(). Given that the device also has the permission to >> access this DMA region, it is possible that a malicious device controlled >> by an attacker can modify the 'valid_dw' field after the if statement but >> before the return statement in msc_data_sz(). Through this way, the >> device So, I *guess* you're assuming that an IOMMU will stop the malicious device from overwriting the kernel code directly, so instead they're reduced to breaking the driver's assumptions about the HW behavior, and that is the reason why patches like this are sent in the first place. This train of thought needs to be explained. Now, if we start defending ourselves against malicious hardware, we need a comprehensive analysis of possible attack vectors and their consequences. I'm not convinced that it needs to be done in the first place, but if it does, it sure needs to be better than grepping for a potential load after validation. >> can bypass the check and supply unexpected data width. >> >> This patch copies the 'valid_dw' field to a local variable and then >> performs the check and the calculation on the local variable to avoid the >> above issue. >> >> Signed-off-by: Wenwen Wang <wang6495@umn.edu> >> --- >> drivers/hwtracing/intel_th/msu.h | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h >> index 9cc8ace..b7d846e 100644 >> --- a/drivers/hwtracing/intel_th/msu.h >> +++ b/drivers/hwtracing/intel_th/msu.h >> @@ -79,10 +79,12 @@ struct msc_block_desc { >> >> static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc) >> { >> - if (!bdesc->valid_dw) >> + u32 valid_dw = bdesc->valid_dw; >> + >> + if (!valid_dw) >> return 0; >> >> - return bdesc->valid_dw * 4 - MSC_BDESC; >> + return valid_dw * 4 - MSC_BDESC; Or, the "malicious device" could just set valid_dw to something within [1; MSC_BDESC/4), pass the check anyway and yield a more interesting result, which may lead to an out-of-bounds access in the buffer reading function. In other words, there's potential for improvement around this function, but this patch misses it. Regards, -- Alex ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-30 11:16 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-19 13:46 [PATCH] intel_th: Fix a missing-check bug Wenwen Wang 2018-10-29 18:39 ` Wenwen Wang 2018-10-30 11:15 ` Alexander Shishkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).