* [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift
@ 2020-02-25 19:53 Qian Cai
2020-02-25 21:40 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Qian Cai @ 2020-02-25 19:53 UTC (permalink / raw)
To: darrick.wong; +Cc: hch, linux-xfs, linux-kernel, Qian Cai
xfs_da3_path_shift() could see state->path.blk[-1] because
state->path.active == 1 is a valid state when it tries to add an entry
to a single dir leaf block and then to shift forward to see if
there's a sibling block that would be a better place to put the new
entry.
UBSAN: Undefined behaviour in fs/xfs/libxfs/xfs_da_btree.c:1989:14
index -1 is out of range for type 'xfs_da_state_blk_t [5]'
Call trace:
dump_backtrace+0x0/0x2c8
show_stack+0x20/0x2c
dump_stack+0xe8/0x150
__ubsan_handle_out_of_bounds+0xe4/0xfc
xfs_da3_path_shift+0x860/0x86c [xfs]
xfs_da3_node_lookup_int+0x7c8/0x934 [xfs]
xfs_dir2_node_addname+0x2c8/0xcd0 [xfs]
xfs_dir_createname+0x348/0x38c [xfs]
xfs_create+0x6b0/0x8b4 [xfs]
xfs_generic_create+0x12c/0x1f8 [xfs]
xfs_vn_mknod+0x3c/0x4c [xfs]
xfs_vn_create+0x34/0x44 [xfs]
do_last+0xd4c/0x10c8
path_openat+0xbc/0x2f4
do_filp_open+0x74/0xf4
do_sys_openat2+0x98/0x180
__arm64_sys_openat+0xf8/0x170
do_el0_svc+0x170/0x240
el0_sync_handler+0x150/0x250
el0_sync+0x164/0x180
Suggested-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Qian Cai <cai@lca.pw>
---
v2: update the commit log thanks to Darrick.
simplify the code.
fs/xfs/libxfs/xfs_da_btree.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 875e04f82541..e864c3d47f60 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -1986,7 +1986,8 @@ static inline int xfs_dabuf_nfsb(struct xfs_mount *mp, int whichfork)
ASSERT(path != NULL);
ASSERT((path->active > 0) && (path->active < XFS_DA_NODE_MAXDEPTH));
level = (path->active-1) - 1; /* skip bottom layer in path */
- for (blk = &path->blk[level]; level >= 0; blk--, level--) {
+ for (; level >= 0; level--) {
+ blk = &path->blk[level];
xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
blk->bp->b_addr);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift
2020-02-25 19:53 [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift Qian Cai
@ 2020-02-25 21:40 ` Christoph Hellwig
2020-02-25 21:55 ` Qian Cai
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2020-02-25 21:40 UTC (permalink / raw)
To: Qian Cai; +Cc: darrick.wong, hch, linux-xfs, linux-kernel
On Tue, Feb 25, 2020 at 02:53:08PM -0500, Qian Cai wrote:
> xfs_da3_path_shift() could see state->path.blk[-1] because
> state->path.active == 1 is a valid state when it tries to add an entry
> to a single dir leaf block and then to shift forward to see if
> there's a sibling block that would be a better place to put the new
> entry.
I think this needs a better explanation. Something like:
In xfs_da3_path_shift() blk can be assigned to state->path.blk[-1] if
state->path.active is 1 (which is a valid state) when it tries to add an
entry > to a single dir leaf block and then to shift forward to see if
there's a sibling block that would be a better place to put the new
entry. This causes a KASAN warning given negative array indices are
undefined behavior in C. In practice the warning is entirely harmless
given that blk is never dereference in this case, but it is still better
to fix up the warning and slightly improve the code.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift
2020-02-25 21:40 ` Christoph Hellwig
@ 2020-02-25 21:55 ` Qian Cai
2020-02-25 22:05 ` Darrick J. Wong
0 siblings, 1 reply; 4+ messages in thread
From: Qian Cai @ 2020-02-25 21:55 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: darrick.wong, linux-xfs, linux-kernel
> On Feb 25, 2020, at 4:40 PM, Christoph Hellwig <hch@infradead.org> wrote:
>
> In xfs_da3_path_shift() blk can be assigned to state->path.blk[-1] if
> state->path.active is 1 (which is a valid state) when it tries to add an
> entry > to a single dir leaf block and then to shift forward to see if
> there's a sibling block that would be a better place to put the new
> entry. This causes a KASAN warning given
s/KASAN/UBSAN/
> negative array indices are
> undefined behavior in C. In practice the warning is entirely harmless
> given that blk is never dereference in this case, but it is still better
> to fix up the warning and slightly improve the code.
Agree. This is better.
Darrick, do you need me to send a v3 for it or you could squash this in?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift
2020-02-25 21:55 ` Qian Cai
@ 2020-02-25 22:05 ` Darrick J. Wong
0 siblings, 0 replies; 4+ messages in thread
From: Darrick J. Wong @ 2020-02-25 22:05 UTC (permalink / raw)
To: Qian Cai; +Cc: Christoph Hellwig, linux-xfs, linux-kernel
On Tue, Feb 25, 2020 at 04:55:56PM -0500, Qian Cai wrote:
>
>
> > On Feb 25, 2020, at 4:40 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >
> > In xfs_da3_path_shift() blk can be assigned to state->path.blk[-1] if
> > state->path.active is 1 (which is a valid state) when it tries to add an
> > entry > to a single dir leaf block and then to shift forward to see if
> > there's a sibling block that would be a better place to put the new
> > entry. This causes a KASAN warning given
>
> s/KASAN/UBSAN/
>
> > negative array indices are
> > undefined behavior in C. In practice the warning is entirely harmless
> > given that blk is never dereference in this case, but it is still better
> > to fix up the warning and slightly improve the code.
>
> Agree. This is better.
>
> Darrick, do you need me to send a v3 for it or you could squash this in?
Please send a v3. The code in v2 looked fine to me.
--D
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-02-25 22:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25 19:53 [PATCH v2] xfs: fix an undefined behaviour in _da3_path_shift Qian Cai
2020-02-25 21:40 ` Christoph Hellwig
2020-02-25 21:55 ` Qian Cai
2020-02-25 22:05 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).