[PATCH 1/4] fs: don't free device in remove callback

[PATCH 1/4] fs: don't free device in remove callback

[Sascha Hauer]

From: Ahmad Fatoum <ahmad@a3f.at>

The probe doesn't allocate the device, so remove shouldn't free it
either. This fixes a use-after-free on barebox shutdown:
Iterating over the list of devices requires that remove callbacks
don't remove the devices. This happened to work so far, because
apparently not much new allocations are going on during barebox
shutdown, but let's do it right.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 fs/fs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index e04cadfe5d..30b835e265 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -679,7 +679,6 @@ static void fs_remove(struct device_d *dev)
 	mntput(fsdev->vfsmount.parent);
 
 	free(fsdev->backingstore);
-	free(fsdev);
 }
 
 struct bus_type fs_bus = {
@@ -759,10 +758,18 @@ static void init_super(struct super_block *sb)
 
 static int fsdev_umount(struct fs_device_d *fsdev)
 {
+	int ret;
+
 	if (fsdev->vfsmount.ref)
 		return -EBUSY;
 
-	return unregister_device(&fsdev->dev);
+	ret = unregister_device(&fsdev->dev);
+	if (ret)
+		return ret;
+
+	free(fsdev);
+
+	return 0;
 }
 
 /**
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/4] fs: Drop unnecessary dput()

[Sascha Hauer]

Calling dput() on the root dentry during unmount time is unnecessary,
the dentry will be removed later in dentry_delete_subtree() anyway.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/fs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/fs.c b/fs/fs.c
index 30b835e265..a6c6f0cc93 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -667,7 +667,6 @@ static void fs_remove(struct device_d *dev)
 	if (fsdev->loop && fsdev->cdev)
 		cdev_remove_loop(fsdev->cdev);
 
-	dput(sb->s_root);
 	dentry_delete_subtree(sb, sb->s_root);
 
 	list_for_each_entry_safe(inode, tmp, &sb->s_inodes, i_sb_list)
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/4] fs: Fix use after free

[Sascha Hauer]

In case of the fs mounted to '/' the root dentry of the mounted
filesystem is the place where it's mounted itself, so sb->s_root
is the same as fsdev->vfsmount.mountpoint. In that case make
sure we only access it before it has been killed in
dentry_delete_subtree().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/fs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index a6c6f0cc93..5784e9c1f3 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -667,14 +667,14 @@ static void fs_remove(struct device_d *dev)
 	if (fsdev->loop && fsdev->cdev)
 		cdev_remove_loop(fsdev->cdev);
 
+	if (fsdev->vfsmount.mountpoint)
+		fsdev->vfsmount.mountpoint->d_flags &= ~DCACHE_MOUNTED;
+
 	dentry_delete_subtree(sb, sb->s_root);
 
 	list_for_each_entry_safe(inode, tmp, &sb->s_inodes, i_sb_list)
 		destroy_inode(inode);
 
-	if (fsdev->vfsmount.mountpoint)
-		fsdev->vfsmount.mountpoint->d_flags &= ~DCACHE_MOUNTED;
-
 	mntput(fsdev->vfsmount.parent);
 
 	free(fsdev->backingstore);
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/4] fs: free unused dentries

[Sascha Hauer]

So far we only ever freed dentries when the filesystem they are on is
unmounted. With this patch we actually trust reference counting and free
the dentries once the reference count hits zero.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/fs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/fs.c b/fs/fs.c
index 5784e9c1f3..824c4e2806 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -1129,6 +1129,9 @@ void dput(struct dentry *dentry)
 		return;
 
 	dentry->d_count--;
+
+	if (!dentry->d_count)
+		dentry_kill(dentry);
 }
 
 struct dentry *dget(struct dentry *dentry)
@@ -2837,7 +2840,6 @@ int mount(const char *device, const char *fsname, const char *pathname,
 		fsdev->vfsmount.mountpoint->d_flags |= DCACHE_MOUNTED;
 	} else {
 		d_root = fsdev->sb.s_root;
-		path.dentry = d_root;
 		mnt_root = &fsdev->vfsmount;
 		fsdev->vfsmount.mountpoint = d_root;
 		fsdev->vfsmount.parent = &fsdev->vfsmount;
-- 
2.28.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Ahmad Fatoum]

Hello Sascha,

On 9/14/20 3:06 PM, Sascha Hauer wrote:
> So far we only ever freed dentries when the filesystem they are on is
> unmounted. With this patch we actually trust reference counting and free
> the dentries once the reference count hits zero.

Unless I revert this patch, I run into a crash doing:

dd if=/dev/zero of=barebox.env count=1
./barebox --image=barebox.env
barebox@barebox sandbox:/ saveenv
saving environment
barebox@barebox sandbox:/ reset
=================================================================
==894761==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000005d70 at pc 0x5617cfde210a bp 0x7ffff89957d0 sp 0x7ffff89957c8
WRITE of size 8 at 0x60e000005d70 thread T0
    #0 0x5617cfde2109 in __list_del /home/a3f/dl/barebox-sbr/include/linux/list.h:112
    #1 0x5617cfde2109 in __list_del_entry /home/a3f/dl/barebox-sbr/include/linux/list.h:135
    #2 0x5617cfde2109 in list_del /home/a3f/dl/barebox-sbr/include/linux/list.h:146
    #3 0x5617cfde2109 in dentry_kill /home/a3f/dl/barebox-sbr/fs/fs.c:653
    #4 0x5617cfde20ad in dentry_kill /home/a3f/dl/barebox-sbr/fs/fs.c:651
    #5 0x5617cfde2284 in dentry_delete_subtree /home/a3f/dl/barebox-sbr/fs/fs.c:668
    #6 0x5617cfde2256 in dentry_delete_subtree /home/a3f/dl/barebox-sbr/fs/fs.c:666
    #7 0x5617cfde2256 in dentry_delete_subtree /home/a3f/dl/barebox-sbr/fs/fs.c:666
    #8 0x5617cfde2256 in dentry_delete_subtree /home/a3f/dl/barebox-sbr/fs/fs.c:666
    #9 0x5617cfde25a2 in fs_remove /home/a3f/dl/barebox-sbr/fs/fs.c:704
    #10 0x5617cfbe8edc in devices_shutdown /home/a3f/dl/barebox-sbr/drivers/base/driver.c:484
    #11 0x5617cfbacbba in shutdown_barebox /home/a3f/dl/barebox-sbr/common/startup.c:422
    #12 0x5617cfd2f2a6 in cmd_reset /home/a3f/dl/barebox-sbr/commands/reset.c:43
    #13 0x5617cfbb8df5 in execute_command /home/a3f/dl/barebox-sbr/common/command.c:72
    #14 0x5617cfbcc1be in run_pipe_real /home/a3f/dl/barebox-sbr/common/hush.c:856
    #15 0x5617cfbcc1be in run_list_real /home/a3f/dl/barebox-sbr/common/hush.c:980
    #16 0x5617cfbcc1be in run_list_real /home/a3f/dl/barebox-sbr/common/hush.c:868
    #17 0x5617cfbca445 in run_list /home/a3f/dl/barebox-sbr/common/hush.c:1097
    #18 0x5617cfbca445 in parse_stream_outer /home/a3f/dl/barebox-sbr/common/hush.c:1725
    #19 0x5617cfbccde9 in run_shell /home/a3f/dl/barebox-sbr/common/hush.c:1948
    #20 0x5617cfbaca6b in run_init /home/a3f/dl/barebox-sbr/common/startup.c:366
    #21 0x5617cfbacb6e in start_barebox /home/a3f/dl/barebox-sbr/common/startup.c:394
    #22 0x5617cfe1d261 in main (/home/a3f/dl/build/barebox/sandbox/barebox+0x3e3261)
    #23 0x7fb6b1fc1cc9 in __libc_start_main ../csu/libc-start.c:308
    #24 0x5617cfba91a9 in _start (/home/a3f/dl/build/barebox/sandbox/barebox+0x16f1a9)

0x60e000005d70 is located 112 bytes inside of 160-byte region [0x60e000005d00,0x60e000005da0)
freed by thread T0 here:
    #0 0x7fb6b2bf3277 in __interceptor_free (/lib/x86_64-linux-gnu/libasan.so.5+0x107277)
    #1 0x5617cfe1d3b0 in barebox_free (/home/a3f/dl/build/barebox/sandbox/barebox+0x3e33b0)
    #2 0x5617cfde21af in dentry_kill /home/a3f/dl/barebox-sbr/fs/fs.c:655

previously allocated by thread T0 here:
    #0 0x7fb6b2bf3628 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x107628)
    #1 0x5617cfe1d396 in barebox_malloc (/home/a3f/dl/build/barebox/sandbox/barebox+0x3e3396)
    #2 0x5617cfd530b5 in xmalloc /home/a3f/dl/barebox-sbr/lib/xfuncs.c:40

SUMMARY: AddressSanitizer: heap-use-after-free /home/a3f/dl/barebox-sbr/include/linux/list.h:112 in __list_del
Shadow bytes around the buggy address:
  0x0c1c7fff8b50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c1c7fff8b60: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c1c7fff8b70: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c1c7fff8b80: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c1c7fff8b90: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
=>0x0c1c7fff8ba0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd
  0x0c1c7fff8bb0: fd fd fd fd fa fa fa fa fa fa fa fa 00 00 00 00
  0x0c1c7fff8bc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1c7fff8bd0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c1c7fff8be0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c1c7fff8bf0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==894761==ABORTING

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  fs/fs.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fs.c b/fs/fs.c
> index 5784e9c1f3..824c4e2806 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -1129,6 +1129,9 @@ void dput(struct dentry *dentry)
>  		return;
>  
>  	dentry->d_count--;
> +
> +	if (!dentry->d_count)
> +		dentry_kill(dentry);
>  }
>  
>  struct dentry *dget(struct dentry *dentry)
> @@ -2837,7 +2840,6 @@ int mount(const char *device, const char *fsname, const char *pathname,
>  		fsdev->vfsmount.mountpoint->d_flags |= DCACHE_MOUNTED;
>  	} else {
>  		d_root = fsdev->sb.s_root;
> -		path.dentry = d_root;
>  		mnt_root = &fsdev->vfsmount;
>  		fsdev->vfsmount.mountpoint = d_root;
>  		fsdev->vfsmount.parent = &fsdev->vfsmount;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Sascha Hauer]

On Tue, Sep 15, 2020 at 07:25:15AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 9/14/20 3:06 PM, Sascha Hauer wrote:
> > So far we only ever freed dentries when the filesystem they are on is
> > unmounted. With this patch we actually trust reference counting and free
> > the dentries once the reference count hits zero.
> 
> Unless I revert this patch, I run into a crash doing:
> 
> dd if=/dev/zero of=barebox.env count=1
> ./barebox --image=barebox.env
> barebox@barebox sandbox:/ saveenv
> saving environment

"saveenv" answers with: "saveenv: No such file or directory". Did you
mean to do a "saveenv /dev/fd0"?

> barebox@barebox sandbox:/ reset
> =================================================================
> ==894761==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000005d70 at pc 0x5617cfde210a bp 0x7ffff89957d0 sp 0x7ffff89957c8

I can't reproduce this here unfortunately.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Ahmad Fatoum]

Hello Sascha,

On 9/15/20 9:12 AM, Sascha Hauer wrote:
> On Tue, Sep 15, 2020 at 07:25:15AM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 9/14/20 3:06 PM, Sascha Hauer wrote:
>>> So far we only ever freed dentries when the filesystem they are on is
>>> unmounted. With this patch we actually trust reference counting and free
>>> the dentries once the reference count hits zero.
>>
>> Unless I revert this patch, I run into a crash doing:
>>
>> dd if=/dev/zero of=barebox.env count=1
>> ./barebox --image=barebox.env
>> barebox@barebox sandbox:/ saveenv
>> saving environment
> 
> "saveenv" answers with: "saveenv: No such file or directory". Did you
> mean to do a "saveenv /dev/fd0"?
> 
>> barebox@barebox sandbox:/ reset
>> =================================================================
>> ==894761==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000005d70 at pc 0x5617cfde210a bp 0x7ffff89957d0 sp 0x7ffff89957c8
> 
> I can't reproduce this here unfortunately.

Sorry, should've been ./barebox --env=barebox.env

I can reproduce it with upstream/next (91a23b64445b8861acbfd96fcc03082b343b0211)
and sandbox_defconfig (+CONFIG_RESET). saveenv; reset results in a segmentation fault.
CONFIG_KASAN=y
CONFIG_MALLOC_LIBC=y
gives the better debugging output.

Cheers,
Ahmad

> 
> Sascha
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Sascha Hauer]

On Tue, Sep 15, 2020 at 10:01:23AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 9/15/20 9:12 AM, Sascha Hauer wrote:
> > On Tue, Sep 15, 2020 at 07:25:15AM +0200, Ahmad Fatoum wrote:
> >> Hello Sascha,
> >>
> >> On 9/14/20 3:06 PM, Sascha Hauer wrote:
> >>> So far we only ever freed dentries when the filesystem they are on is
> >>> unmounted. With this patch we actually trust reference counting and free
> >>> the dentries once the reference count hits zero.
> >>
> >> Unless I revert this patch, I run into a crash doing:
> >>
> >> dd if=/dev/zero of=barebox.env count=1
> >> ./barebox --image=barebox.env
> >> barebox@barebox sandbox:/ saveenv
> >> saving environment
> > 
> > "saveenv" answers with: "saveenv: No such file or directory". Did you
> > mean to do a "saveenv /dev/fd0"?
> > 
> >> barebox@barebox sandbox:/ reset
> >> =================================================================
> >> ==894761==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000005d70 at pc 0x5617cfde210a bp 0x7ffff89957d0 sp 0x7ffff89957c8
> > 
> > I can't reproduce this here unfortunately.
> 
> Sorry, should've been ./barebox --env=barebox.env
> 
> I can reproduce it with upstream/next (91a23b64445b8861acbfd96fcc03082b343b0211)
> and sandbox_defconfig (+CONFIG_RESET). saveenv; reset results in a segmentation fault.
> CONFIG_KASAN=y
> CONFIG_MALLOC_LIBC=y
> gives the better debugging output.

Hm, still can't reproduce, neither with
91a23b64445b8861acbfd96fcc03082b343b0211 nor with current next.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Ahmad Fatoum]

Hello,

On 9/15/20 3:12 PM, Sascha Hauer wrote:
> On Tue, Sep 15, 2020 at 10:01:23AM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 9/15/20 9:12 AM, Sascha Hauer wrote:
>>> On Tue, Sep 15, 2020 at 07:25:15AM +0200, Ahmad Fatoum wrote:
>>>> Hello Sascha,
>>>>
>>>> On 9/14/20 3:06 PM, Sascha Hauer wrote:
>>>>> So far we only ever freed dentries when the filesystem they are on is
>>>>> unmounted. With this patch we actually trust reference counting and free
>>>>> the dentries once the reference count hits zero.
>>>>
>>>> Unless I revert this patch, I run into a crash doing:
>>>>
>>>> dd if=/dev/zero of=barebox.env count=1
>>>> ./barebox --image=barebox.env
>>>> barebox@barebox sandbox:/ saveenv
>>>> saving environment
>>>
>>> "saveenv" answers with: "saveenv: No such file or directory". Did you
>>> mean to do a "saveenv /dev/fd0"?
>>>
>>>> barebox@barebox sandbox:/ reset
>>>> =================================================================
>>>> ==894761==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000005d70 at pc 0x5617cfde210a bp 0x7ffff89957d0 sp 0x7ffff89957c8
>>>
>>> I can't reproduce this here unfortunately.
>>
>> Sorry, should've been ./barebox --env=barebox.env
>>
>> I can reproduce it with upstream/next (91a23b64445b8861acbfd96fcc03082b343b0211)
>> and sandbox_defconfig (+CONFIG_RESET). saveenv; reset results in a segmentation fault.
>> CONFIG_KASAN=y
>> CONFIG_MALLOC_LIBC=y
>> gives the better debugging output.
> 
> Hm, still can't reproduce, neither with
> 91a23b64445b8861acbfd96fcc03082b343b0211 nor with current next.

Just tried on the build server; could you try to run nv autoboot=abort; reset instead?

> 
> Sascha
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/4] fs: free unused dentries

[Ahmad Fatoum]

Hello,

On 9/15/20 3:28 PM, Ahmad Fatoum wrote:
> Just tried on the build server; could you try to run nv autoboot=abort; reset instead?

Small update: THis affects non-sandbox as well. I have observed broken reset on the
stm32mp1.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox