From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f178.google.com ([209.85.192.178]:51196 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751513AbdJGFDD (ORCPT ); Sat, 7 Oct 2017 01:03:03 -0400 Received: by mail-pf0-f178.google.com with SMTP id m63so10460995pfk.7 for ; Fri, 06 Oct 2017 22:03:02 -0700 (PDT) Message-ID: <1507352581.2512.30.camel@dubeyko.com> Subject: Re: [PATCH] hfsplus: fix segfault when deleting all attrs of a file From: Viacheslav Dubeyko To: "Ernesto A." =?ISO-8859-1?Q?Fern=E1ndez?= , linux-fsdevel@vger.kernel.org Cc: Sergei Antonov , Hin-Tak Leung , Al Viro , Christoph Hellwig , Vyacheslav.Dubeyko@wdc.com Date: Fri, 06 Oct 2017 22:03:01 -0700 In-Reply-To: <20171006215222.GA4736@debian.home> References: <20171006215222.GA4736@debian.home> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Fri, 2017-10-06 at 18:52 -0300, Ernesto A. Fernández wrote: > A segmentation fault can be triggered by setting many xattrs to a > file > and then deleting it. The number must be high enough for more than > one > b-tree node to be needed for storage. > I think it could be great to see in the description: (1) The explanation of the reproducing path of the issue; (2) The callstack of the crash and more details about it (if you are able to reproduce the issue, of course). Could you please add these details in the description? > When hfs_brec_remove() is called as part of > hfsplus_delete_all_attrs(), > fd->search_key will not be set to any specific value. It does not > matter > because we intend to remove all records for a given cnid. I am not fully follow to the issue. The hfsplus_delete_all_attrs calls hfsplus_find_attr() before hfsplus_delete_attr(): for (;;) { err = hfsplus_find_attr(dir->i_sb, cnid, NULL, &fd); if (err) { if (err != -ENOENT) pr_err("xattr search failed\n"); goto end_delete_all; } err = __hfsplus_delete_attr(dir, cnid, &fd); if (err) goto end_delete_all; } The hfsplus_find_attr() prepares fd->search_key: if (name) { err = hfsplus_attr_build_key(sb, fd->search_key, cnid, name); if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_rec_by_key); if (err) goto failed_find_attr; } else { err = hfsplus_attr_build_key(sb, fd->search_key, cnid, NULL); if (err) goto failed_find_attr; err = hfs_brec_find(fd, hfs_find_1st_rec_by_cnid); if (err) goto failed_find_attr; } And, finally, __hfsplus_delete_attr() calls the hfs_brec_remove(). I am not follow to the explanation of the issue. Maybe I missed something? Could you share the callstack of the crash andd more details? > > The problem is that hfs_brec_remove() assumes it is being called with > the result of a search by key, not by cnid. The value of search_key > may > be used to update the parent nodes. When no appropriate parent record > is > found, the result is an out of bounds access. > > To fix this, set the value of fd->search_key to the key of the first > record in the node, which is also the key of the corresponding parent > record. > > Signed-off-by: Ernesto A. Fernández > --- >  fs/hfsplus/brec.c | 3 +++ >  1 file changed, 3 insertions(+) > > diff --git a/fs/hfsplus/brec.c b/fs/hfsplus/brec.c > index 754fdf8..dfa60cf 100644 > --- a/fs/hfsplus/brec.c > +++ b/fs/hfsplus/brec.c > @@ -182,6 +182,9 @@ int hfs_brec_remove(struct hfs_find_data *fd) >   >   tree = fd->tree; >   node = fd->bnode; > + > + /* in case we need to search the parent node */ > + hfs_bnode_read_key(node, fd->search_key, 14); The hardcoded value looks really weird. Could you rework this by means of adding some constant with reasonable name? Why 14? >  again: >   rec_off = tree->node_size - (fd->record + 2) * 2; >   end_off = tree->node_size - (node->num_recs + 1) * 2; Thanks, Vyacheslav Dubeyko.