Received-SPF: pass (google.com: domain of linux-kernel+bounces-131721-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 4 Apr 2024 11:25:22 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Jan Kara <jack@suse.cz>
Cc: Kees Cook <keescook@chromium.org>, Christian Brauner <brauner@kernel.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Jeff Layton <jlayton@kernel.org>, Amir Goldstein <amir73il@gmail.com>,
        linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org,
        linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fs: Set file_handle::handle_bytes before referencing
 file_handle::f_handle
Message-ID: <Zg7GYmGQG1S+F4WS@tissot.1015granger.net>
References: <20240403215358.work.365-kees@kernel.org>
 <20240404091900.woh6y2a52o7uo5vx@quack3>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240404091900.woh6y2a52o7uo5vx@quack3>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?HGKPA9IBhEgYNl3VCM6mOcMqmDAwoYIYb/+m4x2xuwwJ7nIjjscFT7Vktx6M?=
 =?us-ascii?Q?SOAMvSuDckHAcmTnwJKp9/DIg6nB3YWug7EDhCjGO6TuxiLGE8nn6f/1HRyS?=
 =?us-ascii?Q?hPl97VGVYIsM61zbXCNTWRvNKdOCWCfFUMIV1VhcjzYdoWBhAwwQFi71nYvu?=
 =?us-ascii?Q?+7g/EZP6CYiYoxMIi6kHgpai/VjXVvNtu0KAHiEwRMKuM5BQcXDPtczWn1lJ?=
 =?us-ascii?Q?X5ZltBtqBYo2b7kG57t9k+LtEHWO8xKqhBtgOrLH/FYs+AaPscz5LmLXWKsD?=
 =?us-ascii?Q?cqN/rWEWIpdTiqsMCAEd/tcB+O19ahQ8wxs/JycnLk+aNTSYrhU+DpgDHyEI?=
 =?us-ascii?Q?BQC29cnRyG4JWH7ClHR/OJ6mEvkgjTKDEvdtnG3j9WSI7lABZ5IhNeed1WOp?=
 =?us-ascii?Q?z6QE9PTqxtAECuZh7z1YjpcMmHFxeZWItLlNyLVvv57bj68oCUXYuqsIcunM?=
 =?us-ascii?Q?gfnbqCe3896OMFEMzfX5dZRXo/HCJOoVtIJgzF9OI0XgCMeI5libVxhm+l5R?=
 =?us-ascii?Q?UjP46NYhGEUDjODy52lMpq01yTXFvWQybLx+8gzAH7IdtsT9K9n+dw0lBkoG?=
 =?us-ascii?Q?G/QnaEe6hbC0g47rcC5DNfL92zQ5cNwxuG4UBQVeaZjfTsxRgUjbia1qQvyB?=
 =?us-ascii?Q?vPMHoGsTEak8ey8w6FwIb2KumcpEXYdJz6cULgb0ijXrcruUZdxo4sHsJ8y4?=
 =?us-ascii?Q?8cwN+t+SWV5xJnm6lwp0vLO+Vg+oLmImgMRrRmCBi90PdNjUmISkJMnckn/g?=
 =?us-ascii?Q?QNykD4FsIS1FeGLEB++egzzcQZvcVR6ZxZ3Ij9xsLFHEda4q3iBLIh0gzK25?=
 =?us-ascii?Q?yqDnXjC19bRtQl2LggWtyQmQOvjR+Hjo6nQZ6yGuF2jdIC4PUaIy9Zw3D2un?=
 =?us-ascii?Q?I+KMP3MeXpfdT60rQnWT8ArydvZ+irQZTtiJV8gGeABuWLoAnpEEFL9kwozz?=
 =?us-ascii?Q?O8OiPxCckCqNhHWlXjV5FgTEctVKDk7g6d4/ATRwyl81oDg83x0raszUE/+C?=
 =?us-ascii?Q?iuGxZeZ6rFIF3+2aDqKCpDiuPh7gAz9Ewj0QgqhE9l8vw5N23RN73WO2fYC7?=
 =?us-ascii?Q?lXHHaahw+4w2TeI8lQkuQ/Dq9s0tEn3DPN54Y/II76eDPQyvQc3qBoaT1SIt?=
 =?us-ascii?Q?kMCU2lVnGP4AA+h4ZHKBUA4rYAikvDT9btYrMIxHF2ebmbDYC24eWW1OPTX2?=
 =?us-ascii?Q?mkBt3P+mzfoVZ36I8ywjdHLCXUdpuHWHaUG2nlHmsbiScHdpqyDrF1+JpUc7?=
 =?us-ascii?Q?9IS5ovsgQz+cywRziww4NiX/zxg1kxapql2MsAV8NuEuvOShRDpUjWY6OzPO?=
 =?us-ascii?Q?QxYWFJHagDJR3uMatHQ2qihnKtMy4W3FECCSjZ0KituRIWOXrX6NJVsNku4W?=
 =?us-ascii?Q?ygD61xziIsP17GRC0jz1I/vx0rgTKr0QlCrQjqrk79eKuhpJa+NN6JNo52OB?=
 =?us-ascii?Q?zwwU7jucLHsz5ZAl4LLFcwZtXk/cxOAPG+EbKmdp+JGbixZzuSuZ4fiGLZ9l?=
 =?us-ascii?Q?QKAoGTTTkUg5KrRMDG4Lts2Wjx+UxqGcwV3NbHdMYgReP9SlvPOzuXa6JFjn?=
 =?us-ascii?Q?pmtHvDBlHSlMDDQjgpnl4vslfZRc/bSUhdO3tfUFDBWFnXzAFVGo7AqiZcl8?=
 =?us-ascii?Q?BQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	uxzTzLhdwylLSiCnbOawbAmZPNHhT1oimIPTY9EDK5U6nKv8zHt1NFapwCn+fcy14JZQ2i76ErHcYybFFGnGc31tzY8Q7Ci95lM3yQErCcYlTm/kjqNfHsANZiY9iWBf8CzpgwK+gJO6mYgEuWV5rcbCCMCdtS10EexoW5/cSdcNKRnxZsWur6PGMYBWZ6Nf4HqMyGsD7fKvnq4eSHzV1xvsRdZvA9+rBEHGly3+x6p7vCUMsMC+ZXWXnraxnWrNDNuz+3Nr13RUn+MLkt3MTtwoorZWPE1bFNI6iiGvcbqzkK7iLR6jN9if9bXDnKnvdmOI27MOS9mtF81F+U3Wr3qQhccAoNtgvUhvKQKPTpP2BnlRbEi8xs8BTWbfJ7/ZVmv2R2LOn75zxQsRUHAQujepke4+E/9PwOISEqXEwTo+HwhhWjizrgp1WKOkMCmGXKIBLHwdBY7c9WKFkL3d88nzDvq17C07zm+E3wJ1nFVOqAQyIU7xekBRtygiwuMZJyq6bgXj0KkyYJUzy7zAyEjALHuERZzb1vK2XEbVga6WGG4c6growVyOjojH/ykXsAbJMLrP4GQWi1Y1/3GEafDK//ebFKeIa2zP7LsBhCc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1955317a-29d8-46c5-c877-08dc54bb7392
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Apr 2024 15:25:25.5695
 (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: yMJ3oWqsug9mfJVMg8LSwOInIxlXh+P1NubnDHabEa3CU/2RX2SCwcmm6Xr10GDH0pjTIzSImwDScu5dkvROHQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR10MB5197
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-04-04_11,2024-04-04_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0
 suspectscore=0 adultscore=0 mlxscore=0 spamscore=0 bulkscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2404010000 definitions=main-2404040107
X-Proofpoint-GUID: fpe8qJIydJc7aLDBJLARVNoie0QpMnaY
X-Proofpoint-ORIG-GUID: fpe8qJIydJc7aLDBJLARVNoie0QpMnaY

On Thu, Apr 04, 2024 at 11:19:00AM +0200, Jan Kara wrote:
> On Wed 03-04-24 14:54:03, Kees Cook wrote:
> > With adding __counted_by(handle_bytes) to struct file_handle, we need
> > to explicitly set it in the one place it wasn't yet happening prior to
> > accessing the flex array "f_handle".
> > 
> > Fixes: 1b43c4629756 ("fs: Annotate struct file_handle with __counted_by() and use struct_size()")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> OK, so this isn't really a functional bug AFAIU but the compiler will
> wrongly complain we are accessing handle->f_handle beyond claimed array
> size (because handle->handle_bytes == 0 at that point). Am I right? If
> that's the case, please add a short comment explaining this (because it
> looks odd we set handle->handle_bytes and then reset it a few lines later).
> With the comment feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> 								Honza

I agree, an in-code comment is needed.

Acked-by: Chuck Lever <chuck.lever@oracle.com>


> > ---
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Chuck Lever <chuck.lever@oracle.com>
> > Cc: Jeff Layton <jlayton@kernel.org>
> > Cc: Amir Goldstein <amir73il@gmail.com>
> > Cc: linux-fsdevel@vger.kernel.org
> > Cc: linux-nfs@vger.kernel.org
> > Cc: linux-hardening@vger.kernel.org
> > ---
> >  fs/fhandle.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 53ed54711cd2..08ec2340dd22 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -40,6 +40,7 @@ static long do_sys_name_to_handle(const struct path *path,
> >  			 GFP_KERNEL);
> >  	if (!handle)
> >  		return -ENOMEM;
> > +	handle->handle_bytes = f_handle.handle_bytes;
> >  
> >  	/* convert handle size to multiple of sizeof(u32) */
> >  	handle_dwords = f_handle.handle_bytes >> 2;
> > -- 
> > 2.34.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

-- 
Chuck Lever