Date:   Wed, 24 Nov 2021 16:01:04 +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 1/3] binder: avoid potential data leakage when copying txn
Message-ID: <20211124130104.GI6514@kadam>
References: <20211123191737.1296541-1-tkjos@google.com>
 <20211123191737.1296541-2-tkjos@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211123191737.1296541-2-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?x4M3AtbhCG6I/YqwOUDWBxLp0i4LPMuZFmVSVUeKdBVNIf8rObZbAy15CWha?=
 =?us-ascii?Q?ZM66s7wOvdzW442gvUXGzdvJ+uEi6qo9KL5Xcua9X24mvxhfcx02loUPD/m3?=
 =?us-ascii?Q?znFWrbcbmLrzvLb6OaF0GZUW4HJo/a5uCYOGcWd0nmFjBcuCxCeooECuGftK?=
 =?us-ascii?Q?PM/SXJ4R+o1zlaVQhM/ZvCuDZfitCGKBsgVNVDVe3GUD0Arp9hZuVAgtKSoR?=
 =?us-ascii?Q?asxdKeshZOfXKDxa2mkMeCV9jU2kE/w7U69w97EhRHToWaSo1zLk4Gaz7BjO?=
 =?us-ascii?Q?q9qf3Yr9vMwJDYSC58+81c6mgx8+rXKs2A+I0fe1i+PPSYZ1X1spcQDzMcFi?=
 =?us-ascii?Q?qnpGBDB/aI4clvNyeg+a3BpyTF3jwtLJNNoiTrqR4UWC/hJcNxoEx8teqooH?=
 =?us-ascii?Q?Kt+lmD9JsmgTLU9HkfTjl+T0vRv/+AYgGDCDoEX7Al7bHbbs/M0LHvcFPfsg?=
 =?us-ascii?Q?VG/B6Kba253kca37ADj44OAXIDgWA4LNL/ZfKTZEB+4qJn9OR0QDTyDyEB+E?=
 =?us-ascii?Q?TW9CwuUuVOLxrrlFjGNL/Sxt5y/UB4Wtoj/FI01T1t/1jdbHMxVPJDrbrV8A?=
 =?us-ascii?Q?AdNzP5hmzPZXdPZ0xQowF1L+MkyJ+lyPuyB2kI4HhbY5eHC8iPtIylUHIAL6?=
 =?us-ascii?Q?fv4j6LbQfOMLqdFm3PiGVqhiGkg77BWgEKZ/rLfFPwEJlhMVj8LKT2mD3Sla?=
 =?us-ascii?Q?XYmXWlx0h7NdeomE+LvrF+TFQTvKMD+f8eMRWcHmIeSiXFPzbOdpGlJDFoT/?=
 =?us-ascii?Q?LPtSwaoN1Zwr8/4XXwCvpC/Bo3hs9eXDHMsEfsnS2j79plFFTm89cv05CznK?=
 =?us-ascii?Q?DprmAPywoKXiULJMQNVCkWYepZRODI0HbhM/Fo0uAyVOH3H02P35uFkqMIDK?=
 =?us-ascii?Q?RaDau9vsZ8uz9InpoteufNT70NgBigTpze3Wnc1xdwxai5qFv/P8N3nY9bov?=
 =?us-ascii?Q?cz2NwDoXXbbeQnqvb8k4KCzUlntiKTb3hE1T5We0fP9eVkkj3VlXd36oJyQ4?=
 =?us-ascii?Q?cAWANPfhyhoD91dudb/DnooSxlUzdxy6O8PO5NShhs3PipWx5YpXK0HRmy/R?=
 =?us-ascii?Q?M86V6fbFRK5clRREiCI/G5GqNV5/Pz2SxIg1t1Sij4ViB2GkPromI3CyYvB2?=
 =?us-ascii?Q?92ri0pfcfi5g0h/L4ZAGt/FzUGUAhKkxGK1xFC++rhSlKFLbd7g5djW/ViUP?=
 =?us-ascii?Q?8NQaw/zYfPHKPgXzcWky4WsWEQu4KumBrkY0wvDP+CXymSlBzRpDK1JEW2+I?=
 =?us-ascii?Q?vLiYpYTOE4FFyrDQw5Jc1qwtaappQJDZ+vuVTwZZL5pt9ECZrrnTsPZJ9o+4?=
 =?us-ascii?Q?cdYpBvN35ZaKS4TLkqfc9/GH4qnUjjB56ki0hbRP1YMRkCE84RPiGpes0Uh3?=
 =?us-ascii?Q?5NBeJo/fMjUDP2Zc60Ui49GKjoBJ5Dkpv8e/uMvobpme89UxVG9FJOvmOCb+?=
 =?us-ascii?Q?Z4V2adiswk9jTzRXrxBodQk74Bj5ud7TnphkY/7cBNZ9VcXaBhJT41VcnfmW?=
 =?us-ascii?Q?82MwlBhVxRXoJ0YjknEYYZSZRgMmGp28oecJwmyMSHZ8RW1knk1eRwgufk/v?=
 =?us-ascii?Q?HuG/0yCWQnYeZIi+NXUwomxSNfufDJY0SW0MfuXHJ6ULGQp4cbIQHFFSn77S?=
 =?us-ascii?Q?q0dc1vpA9st0+ZWGXHwVep/d96xhU98n6FLuNZRPJqKcCesOZ+99gFCxniTw?=
 =?us-ascii?Q?G8StInWXMQRPZ4l8TMhYxSNDI0k=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3cf4f486-704e-41a6-b1b7-08d9af4a8693
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2021 13:01:27.1611
 (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: e0bI8UrEAnCXuFlYZJJsXHvGr/NMfGPHMOGsDiKZ9le0TJMz4TUmZPppv1WyQtW3gfCjL0RW2kzSsqTQfkQ92qKKWwXagagb1zwDzufNNbI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR10MB4468
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10177 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 spamscore=0
 mlxlogscore=952 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111240073
X-Proofpoint-ORIG-GUID: Jyd_dxjLc68gIjm1Da4YKmPqBPrfu5kF
X-Proofpoint-GUID: Jyd_dxjLc68gIjm1Da4YKmPqBPrfu5kF
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:35AM -0800, Todd Kjos wrote:
> Transactions are copied from the sender to the target
> first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
> are then fixed up. This means there is a short period where
> the sender's version of these objects are visible to the
> target prior to the fixups.
> 
> Instead of copying all of the data first, copy data only
> after any needed fixups have been applied.
> 

This patch needs a fixes tag.

So this patch basically fixes the simple part of the info leak and
patch 3 fixes the complicated part.  Have I understood that correctly?

> @@ -2956,7 +2984,11 @@ static void binder_transaction(struct binder_proc *proc,
>  			}
>  			ret = binder_translate_fd_array(fda, parent, t, thread,
>  							in_reply_to);
> -			if (ret < 0) {
> +			if (ret < 0 ||
> +			    binder_alloc_copy_to_buffer(&target_proc->alloc,
> +							t->buffer,
> +							object_offset,
> +							fda, sizeof(*fda))) {
>  				return_error = BR_FAILED_REPLY;
>  				return_error_param = ret;

"ret" is not a negative error code if binder_translate_fd_array()
succeeds but binder_alloc_copy_to_buffer() fails.


>  				return_error_line = __LINE__;
> @@ -3028,6 +3060,19 @@ static void binder_transaction(struct binder_proc *proc,
>  			goto err_bad_object_type;
>  		}
>  	}

regards,
dan carpenter