| Message ID | 20231023-alignment_check-v1-1-2ca5716d5c15@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bus: mhi: host: Add alignment check for event ring read pointer | expand |
On Mon, Oct 23, 2023 at 03:13:06PM +0530, Krishna chaitanya chundru wrote: > Though we do check the event ring read pointer by "is_valid_ring_ptr" > to make sure it is in the buffer range, but there is another risk the > pointer may be not aligned. Since we are expecting event ring elements > are 128 bits(struct mhi_tre) aligned, an unaligned read pointer could lead > to multiple issues like DoS or ring buffer memory corruption. > > So add a alignment check for event ring read pointer. > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > --- > drivers/bus/mhi/host/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > index 499590437e9b..c907bbb67fb2 100644 > --- a/drivers/bus/mhi/host/main.c > +++ b/drivers/bus/mhi/host/main.c > @@ -268,7 +268,7 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, > > static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) > { > - return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; > + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len && addr % 16 == 0; > } > > int mhi_destroy_device(struct device *dev, void *data) > > --- > base-commit: 71e68e182e382e951d6248bccc3c960dcec5a718 > change-id: 20231013-alignment_check-c013f509d24a > > Best regards, > -- > Krishna chaitanya chundru <quic_krichai@quicinc.com> >
On Fri, Oct 27, 2023 at 08:19:44AM -0600, Jeffrey Hugo wrote: > On 10/27/2023 7:09 AM, Manivannan Sadhasivam wrote: > > On Mon, Oct 23, 2023 at 03:13:06PM +0530, Krishna chaitanya chundru wrote: > > > Though we do check the event ring read pointer by "is_valid_ring_ptr" > > > to make sure it is in the buffer range, but there is another risk the > > > pointer may be not aligned. Since we are expecting event ring elements > > > are 128 bits(struct mhi_tre) aligned, an unaligned read pointer could lead > > > > "mhi_tre" got renamed to "mhi_ring_element" > > > > > to multiple issues like DoS or ring buffer memory corruption. > > > > > > So add a alignment check for event ring read pointer. > > > > > > > Since this is a potential fix, you should add the fixes tag and CC stable. > > > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> > > > --- > > > drivers/bus/mhi/host/main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > > > index 499590437e9b..c907bbb67fb2 100644 > > > --- a/drivers/bus/mhi/host/main.c > > > +++ b/drivers/bus/mhi/host/main.c > > > @@ -268,7 +268,7 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, > > > static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) > > > { > > > - return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; > > > + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len && addr % 16 == 0; > > > > How about, > > > > !(addr % 16) > > We are guaranteed that the ring allocation is 16 byte aligned, right? > > I think using "struct mhi_ring_element" instead of "16" would be better. > > I'm also thinking that perhaps doing a bit-wise & with a mask would be > better than the % operator. Not only is that how these alignment checks > seem to normally be done elsewhere, but this check is in a critical patch > for the MHI stack. > Yes, both of your suggestions sounds good to me. Chaitanya, please use below check: !(addr & (sizeof(struct mhi_ring_element) - 1)) - Mani > -Jeff >
On 10/29/2023 12:56 PM, Manivannan Sadhasivam wrote: > On Fri, Oct 27, 2023 at 08:19:44AM -0600, Jeffrey Hugo wrote: >> On 10/27/2023 7:09 AM, Manivannan Sadhasivam wrote: >>> On Mon, Oct 23, 2023 at 03:13:06PM +0530, Krishna chaitanya chundru wrote: >>>> Though we do check the event ring read pointer by "is_valid_ring_ptr" >>>> to make sure it is in the buffer range, but there is another risk the >>>> pointer may be not aligned. Since we are expecting event ring elements >>>> are 128 bits(struct mhi_tre) aligned, an unaligned read pointer could lead >>> "mhi_tre" got renamed to "mhi_ring_element" >>> >>>> to multiple issues like DoS or ring buffer memory corruption. >>>> >>>> So add a alignment check for event ring read pointer. >>>> >>> Since this is a potential fix, you should add the fixes tag and CC stable. >>> >>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> >>>> --- >>>> drivers/bus/mhi/host/main.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c >>>> index 499590437e9b..c907bbb67fb2 100644 >>>> --- a/drivers/bus/mhi/host/main.c >>>> +++ b/drivers/bus/mhi/host/main.c >>>> @@ -268,7 +268,7 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, >>>> static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) >>>> { >>>> - return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; >>>> + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len && addr % 16 == 0; >>> How about, >>> >>> !(addr % 16) >> We are guaranteed that the ring allocation is 16 byte aligned, right? >> >> I think using "struct mhi_ring_element" instead of "16" would be better. >> >> I'm also thinking that perhaps doing a bit-wise & with a mask would be >> better than the % operator. Not only is that how these alignment checks >> seem to normally be done elsewhere, but this check is in a critical patch >> for the MHI stack. >> > Yes, both of your suggestions sounds good to me. > > Chaitanya, please use below check: > > !(addr & (sizeof(struct mhi_ring_element) - 1)) > > - Mani I will update in the next patch. - Krishna Chaitanya. >> -Jeff >>
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c index 499590437e9b..c907bbb67fb2 100644 --- a/drivers/bus/mhi/host/main.c +++ b/drivers/bus/mhi/host/main.c @@ -268,7 +268,7 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) { - return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len && addr % 16 == 0; } int mhi_destroy_device(struct device *dev, void *data)
Though we do check the event ring read pointer by "is_valid_ring_ptr" to make sure it is in the buffer range, but there is another risk the pointer may be not aligned. Since we are expecting event ring elements are 128 bits(struct mhi_tre) aligned, an unaligned read pointer could lead to multiple issues like DoS or ring buffer memory corruption. So add a alignment check for event ring read pointer. Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com> --- drivers/bus/mhi/host/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 71e68e182e382e951d6248bccc3c960dcec5a718 change-id: 20231013-alignment_check-c013f509d24a Best regards,