Date:   Wed, 24 Nov 2021 15:37:19 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Todd Kjos <tkjos@google.com>
Cc:     gregkh@linuxfoundation.org, christian@brauner.io, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
Message-ID: <20211124123719.GG6514@kadam>
References: <20211123191737.1296541-1-tkjos@google.com>
 <20211123191737.1296541-3-tkjos@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211123191737.1296541-3-tkjos@google.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?lEpU+7Q7MGEpsxrc8mmY+6u6848HocHl6fvz9v22Dizfin81FOIrBVtw5JI9?=
 =?us-ascii?Q?BY78dezeWjG4kK9skJbHkdqxfJFq4kBD1w3/b8+ftctSkUK0b7I/XHTrT89k?=
 =?us-ascii?Q?Z9RIXwAddiP3Dux1k+vR40rfKMlUPVOddHs3rWiMMNB+Yjde2+XWbUzpEGTh?=
 =?us-ascii?Q?DoBIbxVqnuh0xPI04lOyAv5/fnEH5vvGP+mWI7fZTMdcCf05Fb4TFMXwuA0u?=
 =?us-ascii?Q?50UGtg63ejn3WxOj/M67DHTNidl5AfbUiYquxN2eNYmkbyn1acFSfxtHjurD?=
 =?us-ascii?Q?g8av0bYLCuGZzkv1LzGx/njWjRsIomWoBj4UbdcfSsnuwB7Ka2x6wkOP9r2q?=
 =?us-ascii?Q?nCJfXMX4Bx4J/mKzqGEKVTOrzFnGS0aVPld1Dv8IWgdDsW1pfZsGvwuC6nGn?=
 =?us-ascii?Q?c+iuOnZ2+N2BpPW2wkPl86fBurAle/NO/xKcBsLCDcdFi2AFNT3mGyp1K+df?=
 =?us-ascii?Q?TqI+kfnCdF5cw+5OS1Vcwm+ANaNUSvkbc4zjPgd+WOShu1KcXFTLCt4KW2sa?=
 =?us-ascii?Q?NcRflDUfvrRWU0PKfVv6ebuqfWqemnoAHXxgVlkzee8vSkuQ6yClPhM/VB1S?=
 =?us-ascii?Q?EYepSuRBp3DntRlCV+dma0hv2YVWdhHcR4EzebIOZ2iVmkTdEP66u3sX+Tz+?=
 =?us-ascii?Q?iyQYG9i+3Fd2HGs5Dj719tDr08HBoMAYRMEwMiEt0z4+CEv4V87rYrgho7M+?=
 =?us-ascii?Q?OzMLlZ7ffoQpGqA2odEdUQ9EEzxsfni43RFbQvLSoNcYB5nHDpWYTVJ+2CbO?=
 =?us-ascii?Q?IPEXHjyMLbrWkJGUuHHuLaSMJNFEG/l3ntT33yaX0QVPuHVk/ATuG3+UMes4?=
 =?us-ascii?Q?Ld69Z/jj+WyxXIzDu0uthAtCGy+6czP0gvndNvgNn1T2UBtOiviu5KRGenIO?=
 =?us-ascii?Q?3cE1bKghhOscawQKfZXH19tJ4CLsT/pWp7KyKj/RwO/euS6yKZ772SH9pMeu?=
 =?us-ascii?Q?CO0SlKd9ODvKRXkON+sjVqguzmUjvexnOVa6wN30qEhDSAbTNKyZ55XcpNsV?=
 =?us-ascii?Q?4CwBXWcF/NHRLW5XFKT6mq2Y0m/vPc3QKrYlqkxtjOh8lcrwinzZFS195UQH?=
 =?us-ascii?Q?mcZFpFdNq/gi5KIwB9dDOHN+aJutDAOsHvXSF6lzIBmLTybm5+vZtL2a/ZQi?=
 =?us-ascii?Q?QGlJACb4ixbdZJq9+4unjm7IGxjWARZINtJ6/HDJDh7AZsCKjOz1yvkXy7id?=
 =?us-ascii?Q?3fov8xojNHjd/7+Iaptd7P+PHwvzqATRlDWE0WADOykdIoWYVC3r1LeMUrEw?=
 =?us-ascii?Q?CdLcRO7egPCRf6Wz0lSIBMUNJ1ExszPfDpugFEnQt7EHMH8J8J1ZrRl/7WRs?=
 =?us-ascii?Q?14lTvLDnl4N1uOY3Gto8+DWR?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 137593b9-b649-4611-73ba-08d9af473466
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2021 12:37:40.9972
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yWqk6smELWL39y5ytPrFKAiq93Q3nphwt2CTNvou5fnmcMITCB2sM5Q4wd6jfY8liSSVOVqufOVtBTC7v4fM8VZdPkfDF1ryrCpCp6ZHtaM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5713
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10177 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 adultscore=0 spamscore=0
 bulkscore=0 suspectscore=0 mlxscore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111240070
X-Proofpoint-ORIG-GUID: NiSeaDl8giGX16aNUfCEap0djvomBQAp
X-Proofpoint-GUID: NiSeaDl8giGX16aNUfCEap0djvomBQAp
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Nov 23, 2021 at 11:17:36AM -0800, Todd Kjos wrote:
> Since we are no longer going to copy the pre-fixup
> data from the target buffer, we need to read
> pre-translated FD array information from the source
> buffer.
> 

The commit message is really misleading.  From the commit message it
sounds like the commit is changing runtime but it's not.  What I want is
a commit message like this:

  This patch is to prepare for an up coming patch where we read
  pre-translated fds from the sender buffer and translate them before
  copying them to the target.  It does not change run time.

  The patch adds two new parameters to binder_translate_fd_array() to
  hold the sender buffer and sender buffer parent.  These parameters let
  us call copy_from_user() directly instead of using
  binder_alloc_copy_from_buffer() which is a cleanup.  Also the patch
  adds some new alignment checks.  Previously the alignment checks would
  have been done in a different place, but this lets us print more
  useful error messages.


> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
>  drivers/android/binder.c | 40 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 571d3c203557..2300fa8e09d5 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2234,15 +2234,17 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
>  }
>  
>  static int binder_translate_fd_array(struct binder_fd_array_object *fda,
> +				     const void __user *u,

I wish we could use sender/target terminology everywhere.  Please change
every place that has "u" or "user" to either "sender" or "target" as
appropriate.

>  				     struct binder_buffer_object *parent,
> +				     struct binder_buffer_object *uparent,
                                                                  ^

>  				     struct binder_transaction *t,
>  				     struct binder_thread *thread,
>  				     struct binder_transaction *in_reply_to)
>  {
>  	binder_size_t fdi, fd_buf_size;
>  	binder_size_t fda_offset;
> +	const void __user *ufda_base;
                           ^

>  	struct binder_proc *proc = thread->proc;
> -	struct binder_proc *target_proc = t->to_proc;
>  
>  	fd_buf_size = sizeof(u32) * fda->num_fds;
>  	if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
> @@ -2266,7 +2268,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
>  	 */>  	fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
>  		fda->parent_offset;
> -	if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) {
> +	ufda_base = (void __user *)uparent->buffer + fda->parent_offset;
> +
> +	if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) ||
> +	    !IS_ALIGNED((unsigned long)ufda_base, sizeof(u32))) {
>  		binder_user_error("%d:%d parent offset not aligned correctly.\n",
>  				  proc->pid, thread->pid);
>  		return -EINVAL;
> @@ -2275,10 +2280,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
>  		u32 fd;
>  		int ret;
>  		binder_size_t offset = fda_offset + fdi * sizeof(fd);
> +		binder_size_t uoffset = fdi * sizeof(fd);
>  
> -		ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
> -						    &fd, t->buffer,
> -						    offset, sizeof(fd));
> +		ret = copy_from_user(&fd, ufda_base + uoffset, sizeof(fd));
>  		if (!ret)
>  			ret = binder_translate_fd(fd, offset, t, thread,
>  						  in_reply_to);

This is something from the original code but if the copy_from_user()
fails, then we just skip that "fd" without informing the user.  It feels
wrong.  Does that not lead to a bug in the target app?


> @@ -2951,6 +2955,8 @@ static void binder_transaction(struct binder_proc *proc,
>  		case BINDER_TYPE_FDA: {
>  			struct binder_object ptr_object;
>  			binder_size_t parent_offset;
> +			struct binder_object user_object;
> +			size_t user_parent_size;
>  			struct binder_fd_array_object *fda =
>  				to_binder_fd_array_object(hdr);
>  			size_t num_valid = (buffer_offset - off_start_offset) /
> @@ -2982,8 +2988,28 @@ static void binder_transaction(struct binder_proc *proc,
>  				return_error_line = __LINE__;
>  				goto err_bad_parent;
>  			}
> -			ret = binder_translate_fd_array(fda, parent, t, thread,
> -							in_reply_to);
> +
> +			/*
> +			 * We need to read the user version of the parent
> +			 * object to get the original user offset
> +			 */
> +			user_parent_size =
> +				binder_get_object(proc, user_buffer, t->buffer,
> +						  parent_offset, &user_object);
> +			if (user_parent_size != sizeof(user_object.bbo)) {
> +				binder_user_error("%d:%d invalid ptr object size: %lld vs %lld\n",

Apparently %lld breaks the build on my .config.  The correct format for
size_t is %zd.

> +						  proc->pid, thread->pid,
> +						  user_parent_size,
> +						  sizeof(user_object.bbo));
> +				return_error = BR_FAILED_REPLY;
> +				return_error_param = -EINVAL;
> +				return_error_line = __LINE__;
> +				goto err_bad_parent;
> +			}
> +			ret = binder_translate_fd_array(fda, user_buffer,
> +							parent,
> +							&user_object.bbo, t,
> +							thread, in_reply_to);

regards,
dan carpenter