On Mon, Oct 09, 2017 at 10:03:50AM -0700, Viacheslav Dubeyko wrote:
> On Sun, 2017-10-08 at 16:46 -0300, Ernesto A. Fernández wrote:
> > On Fri, Oct 06, 2017 at 10:03:01PM -0700, Viacheslav Dubeyko wrote:
> > >
> > > 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?
> > I sent the script I used in a separate mail, would that be enough
> > help?
> > It seems from your review that you already understood all that the
> > callstack can tell you. Maybe I should add the script to the commit
> > message?
> >
>
> The script is the way to reproduce the issue. But the callstack of the
> crash will be the explanation of the symptoms and the environment of
> the issue. Also the callstack provides the opportunity to analyze the
> issue.
All right, I'll include it at the end of the mail.
> > >
> > >
> > > >
> > > > 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);
> > Here hfsplus_attr_build_key will only prepare the key for a search by
> > cnid.
> > We do not yet know the name of the xattr (that's what the NULL is) so
> > it
> > can't be set to anything specific.
> >
> > >
> > > 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?
> > The problem is that hfs_brec_remove() assumes that fd was prepared
> > for a
> > search by key, but it was prepared for a search by cnid. It will fail
> > to
> > find the parent records.
> >
>
> I still cannot follow what the real cause of the issue. This is why I
> requested the callstack of the crash. Could you point out the code line
> in hfs_brec_remove() that is the place of the crash?
Sure, the crash happens in line 219, when the out of bounds write
takes place. But that doesn't tell you much, because the real issue
here is that fd->record has been set to -1 before this call. That value
means that no record has been found, but since that makes no sense (in
this context) the hfs_brec_remove() code acts as if it was an actual
record and tries to write to it.
> I don't see the
> place where hfs_brec_remove() is trying to access the fd->search_key
> directly. It sounds for me that the real trouble is not in the
> hfs_brec_remove().
It is used directly in line 206, explicitly searching the parent node by
key when the search_key has not been set for that purpose. It also uses
it indirectly in line 229, again to search the parent records. In the
second case this just means that we will fail to update the parent record,
which is bad enough. But the first case is the one that will set fd->record
to -1 before going through the code again, this time for the parent node.
When hfs_bnode_write_u16() tries to delete this "-1 record" on the parent,
there is an out of bounds write.
This last thing only happens when we have just deleted the last record of a
bnode.
> Let's imagine that we have trouble with fd->search_key in the
> hfs_brec_remove(). But then it means that the real fix should be
> in hfs_brec_find() or hfs_find_1st_rec_by_cnid(). But, currently, I am
> not convinced by your explanation that the reason is there where you
> did fix it.
I thought about having hfs_brec_find() set fd->search_key to the
same value as fd->key before returning, but I think that would be wrong.
The caller may expect the search_key to remain the same after the search,
and even reuse it. In fact that's what the hfs_brec_remove() function is
trying to do, in a buggy style.
> The struct hfsplus_attr_key is simply the piece of memory:
>
> struct hfsplus_attr_key {
> __be16 key_len;
> __be16 pad;
> hfsplus_cnid cnid;
> __be32 start_block;
> struct hfsplus_attr_unistr key_name;
> } __packed;
>
> And the hfsplus_attr_build_key() initializes this piece of memory:
>
> int hfsplus_attr_build_key(struct super_block *sb, hfsplus_btree_key *key,
> u32 cnid, const char *name)
> {
> int len;
>
> memset(key, 0, sizeof(struct hfsplus_attr_key));
> key->attr.cnid = cpu_to_be32(cnid);
> if (name) {
> int res = hfsplus_asc2uni(sb,
> (struct hfsplus_unistr *)&key->attr.key_name,
> HFSPLUS_ATTR_MAX_STRLEN, name, strlen(name));
> if (res)
> return res;
> len = be16_to_cpu(key->attr.key_name.length);
> } else {
> key->attr.key_name.length = 0;
> len = 0;
> }
>
> /* The length of the key, as stored in key_len field, does not include
> * the size of the key_len field itself.
> * So, offsetof(hfsplus_attr_key, key_name) is a trick because
> * it takes into consideration key_len field (__be16) of
> * hfsplus_attr_key structure instead of length field (__be16) of
> * hfsplus_attr_unistr structure.
> */
> key->key_len =
> cpu_to_be16(offsetof(struct hfsplus_attr_key, key_name) +
> 2 * len);
>
> return 0;
> }
>
> So, even if we haven't name in this structure then the attr.key_name
> will have zero length and the buffer will contain the string is filled
> by zeros. And it's not clear for me how this can be the reason of
> crash? Could you share the callstack of the crash, finally? I still
> don't follow your vision. Please, explain the issue and your fix in
> more clear and detailed way.
I haven't checked the key comparison function used, but what I see in
practice is that when you use a search_key that was set in this way
(prepared only to find the first record with a given cnid) for the purpose
of a search by key, the result is not correct. I get fd->record == -1
in my case, but maybe it can fail in different ways in other situations.
I thought this was clearly a bug in the caller, that had not set the key
to the right value. You seem to think that the search should work anyway,
I believe?
>
> It is possible to do the search by name only or by cnid only.
> Everything should work OK.
I don't think you can search by name only. You can search by key, and the
key has both the name and the cnid. The search by cnid is for finding the
first record with a given cnid, you don't yet know the name so you can't
search by key. This is used by hfsplus_delete_all_attrs() because it
doesn't care about which record is deleted first, it simply deletes all
with a given cnid.
Also you have to explicitly state in the call to __hfs_brec_find() which
one of the two search strategies you will be using. I don't think it is
reasonable to expect __hfs_brec_find() to give a valid result if it is
called with hfs_find_rec_by_key but fd->search_key is only prepared for
a search by cnid.
> And I didn't rememer any troubles
> with hfsplus_delete_all_attrs() on the implementation and testing
> phase. I think you could have some specific environment or maybe the
> code base was changed dramatically. But, anyway, I don't follow your
> vision of the issue.
You are probably right here: I've done all my testing in a fresh
filesystem, and I realize now this segfault would be harder to trigger
if the filesystem has been aged, because the xattrs of different files
may share a single bnode. So deleting all xattrs of a single file would
not necessarily delete a parent bnode, no matter how many xattrs.
This bug would still have consequences though, but they may be more
subtle.
> > >
> > > 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 <ernesto.mnd.fernandez@gmail.
> > > > com>
> > > > ---
> > > > 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?
> > I agree that it's weird, but the number 14 is all over brec.c. It
> > would be
> > more confusing if I used a constant only this one time, so I decided
> > to
> > respect the style that was in use.
> >
> > To be clear, 14 is sizeof(hfs_bnode_desc), that is, the size of the
> > header of the bnode, and the position of the first record. If you
> > find
> > it better, maybe we can change 14 to sizeof(hfs_bnode_desc) every
> > time it appears in the file, but I think it should be a separate
> > patch.
> >
>
> The using of sizeof(hfs_bnode_desc) will be pretty good and clean
> solution.
>
> Thanks,
> Vyacheslav Dubeyko.
>
>
>
Here's the callstack, I hope it helps you:
[ 3550.503259] general protection fault: 0000 [#1] SMP
[ 3550.503587] Modules linked in: nls_utf8 hfsplus loop nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc dlm configfs ppdev snd_pcm snd_timer cirrus snd soundcore ttm pcspkr evdev parport_pc parport pvpanic serio_raw drm_kms_helper button 9pnet_virtio 9pnet drm autofs4 xfs libcrc32c sg sr_mod sd_mod cdrom ata_generic ata_piix libata crc32c_intel psmouse virtio_pci virtio_ring virtio e1000 i2c_piix4 i2c_core scsi_mod floppy
[ 3550.504013] CPU: 0 PID: 1072 Comm: rm Not tainted 4.14.0-rc3+ #16
[ 3550.504013] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
[ 3550.504013] task: ffff880058ab4000 task.stack: ffffc90001630000
[ 3550.504013] RIP: 0010:hfsplus_bnode_write+0xa7/0x1c0 [hfsplus]
[ 3550.504013] RSP: 0018:ffffc90001633c08 EFLAGS: 00010202
[ 3550.504013] RAX: 0005100000000000 RBX: 0000000000000002 RCX: 00000000000000ff
[ 3550.504013] RDX: 0000000000000000 RSI: ffffc90001633c56 RDI: ffff88002972d780
[ 3550.504013] RBP: ffffc90001633c40 R08: ffff88002972d790 R09: 0000000000000000
[ 3550.504013] R10: 0000000000000006 R11: 0000000000000002 R12: 0000000000000002
[ 3550.504013] R13: 0000000000000002 R14: ffff88002972d7e0 R15: ffffc90001633c56
[ 3550.504013] FS: 00007fc27f0c8700(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
[ 3550.504013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3550.504013] CR2: 00007fc27ec40450 CR3: 00000000584c6000 CR4: 00000000000006f0
[ 3550.504013] Call Trace:
[ 3550.504013] hfsplus_bnode_write_u16+0x27/0x30 [hfsplus]
[ 3550.504013] hfsplus_brec_remove+0x117/0x170 [hfsplus]
[ 3550.504013] __hfsplus_delete_attr+0x94/0xf0 [hfsplus]
[ 3550.504013] hfsplus_delete_all_attrs+0x4a/0xb0 [hfsplus]
[ 3550.504013] hfsplus_delete_cat+0x1f5/0x300 [hfsplus]
[ 3550.504013] hfsplus_unlink+0x82/0x1e0 [hfsplus]
[ 3550.504013] ? __inode_permission+0x44/0xc0
[ 3550.504013] vfs_unlink+0xf1/0x180
[ 3550.504013] do_unlinkat+0x25f/0x2e0
[ 3550.504013] SyS_unlinkat+0x1b/0x30
[ 3550.504013] entry_SYSCALL_64_fastpath+0x1e/0xa9
[ 3550.504013] RIP: 0033:0x7fc27ebe632d
[ 3550.504013] RSP: 002b:00007fff036b4008 EFLAGS: 00000202 ORIG_RAX: 0000000000000107
[ 3550.504013] RAX: ffffffffffffffda RBX: 00000000025cd2f0 RCX: 00007fc27ebe632d
[ 3550.504013] RDX: 0000000000000000 RSI: 00000000025cc0c0 RDI: ffffffffffffff9c
[ 3550.504013] RBP: 00000000025cd420 R08: 0000000000000003 R09: 0000000000000000
[ 3550.504013] R10: 00007fff036b3dd0 R11: 0000000000000202 R12: 00000000025cc030
[ 3550.504013] R13: 00000000025cd3f8 R14: 0000000000000000 R15: 0000000000000000
[ 3550.504013] Code: c1 fb 06 48 c1 e3 0c 48 01 d8 49 63 dd 48 01 d0 48 83 fb 08 73 26 f6 c3 04 0f 85 04 01 00 00 48 85 db 74 44 41 0f b6 0f f6 c3 02 <88> 08 74 39 41 0f b7 4c 1f fe 66 89 4c 03 fe eb 2c 49 8b 0f 48
[ 3550.504013] RIP: hfsplus_bnode_write+0xa7/0x1c0 [hfsplus] RSP: ffffc90001633c08
[ 3550.540759] ---[ end trace 142de398139577f1 ]---
