From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965746AbcAZSTD (ORCPT ); Tue, 26 Jan 2016 13:19:03 -0500 Received: from mail-pa0-f48.google.com ([209.85.220.48]:36089 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964826AbcAZSS7 (ORCPT ); Tue, 26 Jan 2016 13:18:59 -0500 Message-ID: <1453832336.2633.10.camel@slavad-ubuntu-14.04> Subject: Re: [patch] hfs: fix hfs_readdir() From: Viacheslav Dubeyko To: Dan Carpenter Cc: Chengyu Song , Andrew Morton , David Howells , Al Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org Date: Tue, 26 Jan 2016 10:18:56 -0800 In-Reply-To: <20160126092611.GD15717@mwanda> References: <20160126092611.GD15717@mwanda> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2016-01-26 at 12:26 +0300, Dan Carpenter wrote: > I was looking through static analysis warnings and we seem to be copying > garbage into &rd->key. This goes back to before the start of git... > > Signed-off-by: Dan Carpenter > --- > Not tested. Please review carefully. > > diff --git a/fs/hfs/dir.c b/fs/hfs/dir.c > index 70788e0..66485d7 100644 > --- a/fs/hfs/dir.c > +++ b/fs/hfs/dir.c > @@ -163,7 +163,7 @@ static int hfs_readdir(struct file *file, struct dir_context *ctx) > rd->file = file; > list_add(&rd->list, &HFS_I(inode)->open_dir_list); > } > - memcpy(&rd->key, &fd.key, sizeof(struct hfs_cat_key)); > + memcpy(&rd->key, &fd.key->cat, sizeof(struct hfs_cat_key)); The field "key" is union: 164 typedef union hfs_btree_key { 165 u8 key_len; /* number of bytes in the key */ 166 struct hfs_cat_key cat; 167 struct hfs_ext_key ext; 168 } hfs_btree_key; The struct hfs_cat_key is the biggest item. So, size of this structure is dominating in the union: 157 struct hfs_ext_key { 158 u8 key_len; /* number of bytes in the key */ 159 u8 FkType; /* HFS_FK_{DATA,RSRC} */ 160 __be32 FNum; /* The File ID of the file */ 161 __be16 FABN; /* allocation blocks number*/ 162 } __packed; 149 struct hfs_cat_key { 150 u8 key_len; /* number of bytes in the key */ 151 u8 reserved; /* padding */ 152 __be32 ParID; /* CNID of the parent dir */ 153 struct hfs_name CName; /* The filename of the entry */ 154 } __packed; because: 27 #define HFS_NAMELEN 31 /* maximum length of an HFS filename */ 87 struct hfs_name { 88 u8 len; 89 u8 name[HFS_NAMELEN]; 90 } __packed; If we are using sizeof(struct hfs_cat_key) then it looks like that we could potentially miss one byte of the union during catalog key copying. But if we will copy struct hfs_ext_key then we will copy some amount of "garbage" anyway. So, I don't think that it's good fix of the issue. What do you think? Another worry could be the "search_key" field of the struct hfs_find_data. Thanks, Vyacheslav Dubeyko.