Message ID | 20231023-alignment_check-v1-1-2ca5716d5c15@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:ce89:0:b0:403:3b70:6f57 with SMTP id p9csp1178172vqx; Mon, 23 Oct 2023 02:44:16 -0700 (PDT) X-Google-Smtp-Source: AGHT+IERV70sBjCvtWyUccfI5MoO3L1eBzN86KEd/b2nzyZU48Dr5LCNqjx/cVVEXGPGTxuWc0d8 X-Received: by 2002:a05:6808:1493:b0:3a7:4161:44ee with SMTP id e19-20020a056808149300b003a7416144eemr9446403oiw.6.1698054256002; Mon, 23 Oct 2023 02:44:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698054255; cv=none; d=google.com; s=arc-20160816; b=UhIlZU3tj9z4CyuwaGm685MrWiPTU29BYuKPu3XETUInDQ9rSckZNFel8POKjk5nDX dRV9doIcrrGqELMmZGExur8wIM9BYBLVfG3gGTL796rfv4jKwJ5rY73s5ZXDVrfKrVra uQHVAEa++E5+Bemlb+Y7//zPdmcNILVb0OoULsSpA1GOYuuIl7gDwKCeNIPkqOOxkW0H ZX8hKBrjo3vU21u74GsW4pA+3uvrPflYOJBMqQTb3wlgV/1Ysp8Bzj4BPS6VBxvk/RAl E55ROrqQtTbi5PZH2YMLN7gNDYcRQz2qBOqMa9FnPPHtjGFNj0lIuBQCEpRBUODmgTtc +lBA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:message-id:content-transfer-encoding :mime-version:subject:date:from:dkim-signature; bh=qX2IpiVetfNrVJqIeQTFFJqLBVuxvBnZSADd9eG8WYE=; fh=sdf3bDv1sZXvCPVlWzWDi5gAunlutpEQJRV7CbBA4pI=; b=BuB7FFiQ/0j0veCCujl9o7VO4Wp9ZzZJ6J3eUkfJUoqwSXLVi9xTMzmYmbp4DTvt57 J8S7F9+m0cSska+ATWWI1gTz8IfPxxYceC8d+zOYbyf6UOqxMxS5O5FU71Pl1jS449ft lWsHyNEpy+VTK3A4Tc2JHY0/qM7fbjNP8VP5clPJCIEMRAdJemclfK2YBXEbouQWtejb 391l18RV3qsBVPPv/rhcXjI7HiwEx53chcRL0DW0MF54qxo5BUxERM7b9EBARTutT8YR P0Q51qTGVPtqJHAEwiO/AKVSqHTln2XBFdCfXBPdx5Wu1wEGcAPsyv28cf92qK/RCKSH p0Cg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=W7GrQYWr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from morse.vger.email (morse.vger.email. [2620:137:e000::3:1]) by mx.google.com with ESMTPS id b16-20020a637150000000b0058a09ed5ca2si6190928pgn.313.2023.10.23.02.44.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Oct 2023 02:44:15 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=W7GrQYWr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id EB3478042D19; Mon, 23 Oct 2023 02:44:13 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230022AbjJWJnp (ORCPT <rfc822;aposhian.dev@gmail.com> + 27 others); Mon, 23 Oct 2023 05:43:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53214 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229589AbjJWJnm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 23 Oct 2023 05:43:42 -0400 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2F272A4; Mon, 23 Oct 2023 02:43:40 -0700 (PDT) Received: from pps.filterd (m0279869.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39N7Sl96004979; Mon, 23 Oct 2023 09:43:29 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : date : subject : mime-version : content-type : content-transfer-encoding : message-id : to : cc; s=qcppdkim1; bh=qX2IpiVetfNrVJqIeQTFFJqLBVuxvBnZSADd9eG8WYE=; b=W7GrQYWrK7kdGTOeprjlOLQGwZHbnIPvtpTYkXMJ5fr1y1n7oGSyki+YupEIqK4GyOj4 4O4ATlUyPjLdSHoevXecwF8eEcj+zlmvf3tPEVuqxWmnR8DL6U2bidKsTmlT2wEcmdni Gf26eFuHxF0jdPIqB91i7KwEUXhhltuLzOv/FahUBmnYzi+/SM/R1R3356RmFwyEmJ05 U4h+GpAWJoC4ad1DBuR9fh3YhZuWnStzeG8XqLnC+5tmFb5tLR6hBn2232uujm5eAh7b CYpqIlcxwGTrgmilABchXogqbvdoA2svpDgzAEKjF7TIpZo6WDIBvV2SVavxf0lIGFB1 iA== Received: from nalasppmta03.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3tv5ndupap-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 23 Oct 2023 09:43:28 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA03.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 39N9hRh6013417 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 23 Oct 2023 09:43:27 GMT Received: from hu-krichai-hyd.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.39; Mon, 23 Oct 2023 02:43:24 -0700 From: Krishna chaitanya chundru <quic_krichai@quicinc.com> Date: Mon, 23 Oct 2023 15:13:06 +0530 Subject: [PATCH] bus: mhi: host: Add alignment check for event ring read pointer MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-ID: <20231023-alignment_check-v1-1-2ca5716d5c15@quicinc.com> X-B4-Tracking: v=1; b=H4sIACtANmUC/x2MWwqAIBAAryL7neCjPuoqESHbWktloRGBdPekz xmYyZAoMiXoRIZINyc+QgFdCcDFhZkkT4XBKGO10la6jeewU7hGXAhXiUX6RrWTqR2U6ozk+fm P/fC+HwxYO+FhAAAA To: Manivannan Sadhasivam <mani@kernel.org> CC: <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <quic_vbadigan@quicinc.com>, <quic_ramkri@quicinc.com>, <quic_skananth@quicinc.com>, <quic_parass@quicinc.com>, Krishna chaitanya chundru <quic_krichai@quicinc.com> X-Mailer: b4 0.13-dev-83828 X-Developer-Signature: v=1; a=ed25519-sha256; t=1698054204; l=1344; i=quic_krichai@quicinc.com; s=20230907; h=from:subject:message-id; bh=07vhrQIHUHsNMbpycpffRwOJ06Rlyx3FlRgHXf2kqMM=; b=VVkOs1tRmVi0Mknh7IuNBSLQsMLHWqRd0EnFDT3RqhNgi0YspG+Rd+FKnCeuhUjMmUkchFJG6 MJHfvo5ZEdTBhweh8x9izTlJNCwwFRuzAUduJM5ORFhSS+eKEapJiPT X-Developer-Key: i=quic_krichai@quicinc.com; a=ed25519; pk=10CL2pdAKFyzyOHbfSWHCD0X0my7CXxj8gJScmn1FAg= X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01b.na.qualcomm.com (10.46.141.250) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: EawWRLqZJI-lZF3c_1622c3Ev9i5ErU7 X-Proofpoint-ORIG-GUID: EawWRLqZJI-lZF3c_1622c3Ev9i5ErU7 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-23_07,2023-10-19_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxlogscore=847 bulkscore=0 adultscore=0 impostorscore=0 mlxscore=0 phishscore=0 priorityscore=1501 clxscore=1015 suspectscore=0 spamscore=0 lowpriorityscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001 definitions=main-2310230083 X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Mon, 23 Oct 2023 02:44:14 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1780538939289338976 X-GMAIL-MSGID: 1780538939289338976 |
Series |
bus: mhi: host: Add alignment check for event ring read pointer
|
|
Commit Message
Krishna chaitanya chundru
Oct. 23, 2023, 9:43 a.m. UTC
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,
Comments
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 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) - Mani > } > > 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 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. -Jeff
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)