Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 19 Sep 2022 15:04:19 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     eadavis@sina.com
Cc:     almaz.alexandrovich@paragon-software.com, kbuild-all@lists.01.org,
        linux-kernel@vger.kernel.org, lkp@intel.com, llvm@lists.linux.dev,
        nathan@kernel.org, ndesaulniers@google.com, ntfs3@lists.linux.dev,
        syzbot+c4d950787fd5553287b7@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com, trix@redhat.com
Subject: Re: [PATCH v3] fs/ntfs3: add a boundary check for EA_FULL
Message-ID: <Yyhaw8iBPulIO0Pp@kadam>
References: <YyhHowvgEarPqONR@kadam>
 <20220919111954.283125-1-eadavis@sina.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220919111954.283125-1-eadavis@sina.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8XMOqpo0T/i0lDnhXzXUWYqW3N8Msx0D09BIi271Bw1HFyieOtpf3vr8Yq22?=
 =?us-ascii?Q?JVV2ozoNA8zzO5fPWsjTWZozxKhODJ2Onj6AwFnyUEJroY45Us7yRZE3EGn3?=
 =?us-ascii?Q?NEPIkoPERlqs2XNmrnJz+6bbOQQ87FUdM/EjGHR67UjjKGqwYK7iPlcELj9z?=
 =?us-ascii?Q?+i/vNcgWBuLb+r9T4ZeHZ733o42pEn9bc7BFmumEtKCArEwQqquw+a42GlOP?=
 =?us-ascii?Q?vw7LyqKxu8+NSsNjivhRcO6Yarxz7WF4M2P3IEOLGqlogSdK9DSvc6Q26xt3?=
 =?us-ascii?Q?YkIpiCqERgf73Z/GLmdAVHs7A58wWGLV8HiNxGv15BC2U5f+H8/60Xfgu3M+?=
 =?us-ascii?Q?sUlz2aHpj3DWkMci2wizjirf0bX2JgWL9HXZRT3YLKuyxGEcy2tmD48sleB0?=
 =?us-ascii?Q?QS9lGqA45vn5eh0rWOcSPEHMsftTVrxh5A5kvALnJc5Qu25Y9GcMuiPgPuDq?=
 =?us-ascii?Q?xJ9lHzg4WRDvjs0m5aXk61EVvPgU31ZaswNh+C4utRuSItAE4W4UBPmZ9u9I?=
 =?us-ascii?Q?l4onsflfrVX4Xqu7ImSHUL/KfMadZuDEJItWpeRh6spTo6K36MpokijAjreY?=
 =?us-ascii?Q?VYsEDEvboJUlylflom59Hmz4PmNSXIqRUbHRstnnQikyH9yEvQxDJqOu0BaB?=
 =?us-ascii?Q?c5dPcbfxORxln8JFkdpMKKaH2ZUDqTMSjvqVSGhICqaOhx4wgaZQ6SuYTRtq?=
 =?us-ascii?Q?etyw/T1DgiKg47xLW4ymLhLLna1w/P9lDxB68L7bDUQzfRfhbRXlBvogS/Bq?=
 =?us-ascii?Q?f2Imyo20Rcp4oULLywmsAMnuXOFPjdEpC8BLsodf9ERsnocpfmaXuqj7NG6w?=
 =?us-ascii?Q?r8upUulhNb4xSe+t/M54I2SQ8ju2xmaUy/MhnETHVfyiYBTsuV9AjZMQPych?=
 =?us-ascii?Q?iFrbfKjI7W901sdI1zci7+XPf6J8So4GEHKYCgLdfH0PHLYFjMEPejkHCdEo?=
 =?us-ascii?Q?B6lMTy10SA0YZ1H5idUQTwMe1byp3RHYCvj8DivB1cSMcCytC26fXDRXKhpT?=
 =?us-ascii?Q?0r3OiDv8EDdE+zN658M9pN4TzYFYOkdL5HbbofdsCRL8obgu4ITb1QTj1tSg?=
 =?us-ascii?Q?oUhSez7fwaTzOECUl3lbXXLJ6tWePecNsThpUZRtRscQegau2saz5iYcC3r/?=
 =?us-ascii?Q?TFSYJrq5CpGqUfsy+5dDcQhK2XJFAV/rAs2ZEgh9yRusvjJ4mIe+QSxjcDCP?=
 =?us-ascii?Q?Io9j76RZB0+7VuMM39S2lRcbYijztURySIbjJ5RPcIi2ss9jy2Ozvv/Fx2aL?=
 =?us-ascii?Q?Sb3NUcJ7UkMx2+6TSLHHZpM6E5M0zlQvqdSBGyMKU4oGuEmH7HIaq36y+aJi?=
 =?us-ascii?Q?V0wCf4mmhkPV3HmbMZhWwWqexG5XP5rLs5+edt0D16i19lXB5TMjuGJA53v1?=
 =?us-ascii?Q?g1PuHE68kkXU77mneSFUCxhSjlrDzvJVL0Ylgjul/dFQjhclPpwdXsu3Nyuc?=
 =?us-ascii?Q?hY69faU31Pj54+1+JotP7Te0KiSU0Vb1CURouyw5tA149uf8FsIb6UIw0T3k?=
 =?us-ascii?Q?RelgmX+nEoZBgsT4tVDO5iTd141BiSreE69kvjR7qRVnj1FzJcQ/HkBpHUYL?=
 =?us-ascii?Q?rrbSHqbW0NKoIWXsPgx4d4cNLAghUTeMIU+j2aRv2gh2EbM3JIsliQog/XrA?=
 =?us-ascii?Q?cA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6073fdd4-6066-41a4-f705-08da9a3723ac
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Sep 2022 12:04:44.1731
 (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: s2cDxR3Y5KyM5+EFSi1tFSECkE5BMIXdGg5RzQNkQ+he3AVawCaaT0q+LEvH01P5H/optDyYZ6CvIFxSJ9yEZp/nUjYcaWEXNLA7YU3v4ec=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB6826
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-19_05,2022-09-16_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 phishscore=0 bulkscore=0 malwarescore=0 spamscore=0 suspectscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2209190081
X-Proofpoint-GUID: 4Rpih3-1iIM0uHw-JzpPw2MnaVqsSofh
X-Proofpoint-ORIG-GUID: 4Rpih3-1iIM0uHw-JzpPw2MnaVqsSofh
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

This patch is useful and shows what the bug is however the fix needs
some additional work.

On Mon, Sep 19, 2022 at 07:19:54PM +0800, eadavis@sina.com wrote:
> From: Edward Adam Davis <eadavis@sina.com>
> 
> the root cause is: 
> The remaining space after the offset is less than the space needed to 
> accommodate the next EA_FULL struct.

This commit message is not good and does not explain what how the
problem appears to the user.  Don't start the commit message in the
middle of a sentence.

> 
> Link: https://syzkaller.appspot.com/bug?extid=c4d950787fd5553287b7  
> Reported-by: syzbot+c4d950787fd5553287b7@syzkaller.appspotmail.com
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Edward Adam Davis <eadavis@sina.com>
> ---
> Changes in v3:
>   Add Suggested-by: and fix the syntax err.

This is not what I suggested... There several problems with this patch.

1) If we call find_ea() if "bytes" set to a number in 1-7 range then
   then the unpacked_ea_size() is an out of bounds read.
2) The *off + unpacked_ea_size() can have an integer overflow.
3) The math in "next_len + ea->name_len + le16_to_cpu(ea->elength)" is
   not correct.  It should use unpacked_ea_size() instead.  (The math
   is different, this is a bug an not a style complaint).
4) My one style complaint is that "8" in "next_off + 8" is a magic
   number.

So maybe we could separate out the issue with the buffer overflow from
the integer overflow.

Patch 1:  Fix buffer overflow:

Add "if (bytes - *off < sizeof(*ea))" before the call to
"ea = Add2Ptr(ea_all, *off);".  Then remove the "!bytes" test.  That
test is wrong and useless.  Also remove the if (next_off >= bytes) test.
That test is also insufficient and no longer required.

Patch 2: Fix the integer overflow bug.

-		next_off = *off + unpacked_ea_size(ea);
+		next_off = size_add(*off, unpacked_ea_size(ea));

We also need to change the types of next_off and bytes to size_t for the
size_add() to be useful.  I forgot about that last time I sent this.

After both patches are applied the code will look like below.

regards,
dan carpenter


diff --git a/fs/ntfs3/xattr.c b/fs/ntfs3/xattr.c
index 7de8718c68a9..bf53ed96b03f 100644
--- a/fs/ntfs3/xattr.c
+++ b/fs/ntfs3/xattr.c
@@ -41,17 +41,24 @@ static inline size_t packed_ea_size(const struct EA_FULL *ea)
  *
  * Assume there is at least one xattr in the list.
  */
-static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
+static inline bool find_ea(const struct EA_FULL *ea_all, size_t bytes,
 			   const char *name, u8 name_len, u32 *off)
 {
+	const struct EA_FULL *ea;
+	size_t next_off;
+
 	*off = 0;
 
-	if (!ea_all || !bytes)
+	if (!ea_all)
 		return false;
 
+
 	for (;;) {
-		const struct EA_FULL *ea = Add2Ptr(ea_all, *off);
-		u32 next_off = *off + unpacked_ea_size(ea);
+		if (bytes - *off < sizeof(*ea))
+			return false;
+
+		ea = Add2Ptr(ea_all, *off);
+		next_off = size_add(*off, unpacked_ea_size(ea));
 
 		if (next_off > bytes)
 			return false;
@@ -61,8 +68,6 @@ static inline bool find_ea(const struct EA_FULL *ea_all, u32 bytes,
 			return true;
 
 		*off = next_off;
-		if (next_off >= bytes)
-			return false;
 	}
 }