From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6696BC433DB for ; Tue, 5 Jan 2021 11:06:21 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 075D8229EF for ; Tue, 5 Jan 2021 11:06:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 075D8229EF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=oracle.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=JA6Nq4XYR/mESDbTEKM7JZLAZ2XTplNcpNQXqsEGsxw=; b=nhAraf9NoyBjG15DgNJziOAS8 11qVQS90hVYdaX6UBJUz+YKP41PB9KqNsVUGWXvfCVAc11EcVwSFuUidL4goIC0wMSIc/nuXa9is6 c1DvFWGe5rVz1ekvhTJ4u6gYPpN7YYtL+mxCmy+UE9YVKeVvQYyqOUyKb2ymc6InTCOYNuC19gGfA FPFZoRaCqVjmckaYeJbp5yt/VhRIGfirIxQA5LFtzzmGZFirQ2lPlOGUDysGaoiN4X2G/ypNC7hdo EfB2XfwWoqsoFWwbb/OaUUmXTa9Kq4tWCS30A5rH71UleL0z1YR/H3w1ADQxf0wc0VyVkfPHLYp2W 5MnZsgeMw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kwk8k-0002dg-CM; Tue, 05 Jan 2021 11:04:38 +0000 Received: from aserp2120.oracle.com ([141.146.126.78]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kwk8f-0002cH-Mn; Tue, 05 Jan 2021 11:04:35 +0000 Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 105Ax2xw087539; Tue, 5 Jan 2021 11:04:21 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=corp-2020-01-29; bh=/7lHIJeWLo852eGuhRbc2FpzgftlCkBt+r8NhhFErlE=; b=knWsgT7LeGOmtL3vZvsHtBUku7iRLDLVeI297AUzlgtT8wT9OA3enEPytLYhlRzpF4+s OAxuP41d8KjTBAee8OWauYSEy45mp6uHgYrOqG6S2v1B9hRzz0sN9ZFdaJiC1vQGBDvp WlxVlyJAXzFVa4m7V4tAnfOcKqSNnHSDxua7QXAviiskbTgjNT0hqz6QCJKx67Xy/Lne tie6Uk1QLo65HTliBkvYWGuFLSdfbG67OYqw2qG9n2KaGJZiCyiMFYyrcjwmJR2nkOL8 gyLqY68GcxV7ljfgOEtV7wnHvSqSSZy05KiPnWlim1vQ1QlFOAOOaEwi7NLPgfj+YsPh dg== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2120.oracle.com with ESMTP id 35tgskra4a-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 05 Jan 2021 11:04:21 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 105B0kcV068669; Tue, 5 Jan 2021 11:02:20 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserp3020.oracle.com with ESMTP id 35v1f8fam7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 05 Jan 2021 11:02:20 +0000 Received: from abhmp0019.oracle.com (abhmp0019.oracle.com [141.146.116.25]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 105B2Fra007016; Tue, 5 Jan 2021 11:02:15 GMT Received: from kadam (/102.36.221.92) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 05 Jan 2021 11:02:14 +0000 Date: Tue, 5 Jan 2021 14:01:41 +0300 From: Dan Carpenter To: Phil Elwell Subject: Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling Message-ID: <20210105110140.GW2809@kadam> References: <20210104120929.294063-1-phil@raspberrypi.com> <20210104120929.294063-2-phil@raspberrypi.com> <20210104183134.GV2809@kadam> <989ef44f-2afe-5147-1277-74df56797a4c@raspberrypi.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <989ef44f-2afe-5147-1277-74df56797a4c@raspberrypi.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9854 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0 suspectscore=0 spamscore=0 bulkscore=0 adultscore=0 mlxscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101050068 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9854 signatures=668683 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 spamscore=0 malwarescore=0 phishscore=0 impostorscore=0 bulkscore=0 clxscore=1015 priorityscore=1501 lowpriorityscore=0 adultscore=0 suspectscore=0 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101050068 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210105_060433_885184_1995B53B X-CRM114-Status: GOOD ( 32.96 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devel@driverdev.osuosl.org, Arnd Bergmann , Greg Kroah-Hartman , bcm-kernel-feedback-list@broadcom.com, linux-arm-kernel@lists.infradead.org, Nicolas Saenz Julienne , linux-rpi-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Jan 04, 2021 at 07:26:42PM +0000, Phil Elwell wrote: > On 04/01/2021 18:31, Dan Carpenter wrote: > > On Mon, Jan 04, 2021 at 12:09:27PM +0000, Phil Elwell wrote: > > > The addition of the local 'userdata' pointer to > > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor > > > WAITING modes are used, in which case the value provided by the > > > caller is replaced with a NULL. > > > > > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations") > > > > > > Signed-off-by: Phil Elwell > > > --- > > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > index f500a7043805..2a8883673ba1 100644 > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance, > > > struct vchiq_service *service; > > > struct bulk_waiter_node *waiter = NULL; > > > bool found = false; > > > - void *userdata = NULL; > > > + void *userdata; > > > int status = 0; > > > int ret; > > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance, > > > "found bulk_waiter %pK for pid %d", waiter, > > > current->pid); > > > userdata = &waiter->bulk_waiter; > > > + } else { > > > + userdata = args->userdata; > > > > "args->userdata" is marked as a user pointer so we really don't want to > > mix user and kernel pointers here. Presumably this opens up a large > > security hole. > > It's an opaque, pointer-sized token that only exists to bereturned to userspace (or not, > without this patch) - it's hard to see that as a security hole. I was assuming the bug here was a NULL dereference... Apparently that's not the case? The commit message needs to be updated to be more clear about how the bug looks like to the user. Are we using the "&waiter->bulk_waiter" as a "token to be returned to userspace" as well? It looks like maybe it is in vchiq_put_completion(). That defeats KASLR and is a different sort of security problem. Mixing __user pointers and regular pointers is dangerous and has lead to security problems in this driver in the past. But also mixing mixing tokens with pointers just makes the code hard to read. Instead of undoing Arnd's work where he split the user space and kernel pointers apart we should go ahead and spit it up even more. At least add a giant FIXME comment and an item in the TODO list so we don't forget to do this before removing the code from staging. regards, dan carpenter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel