Message ID | 1425903787-15230-1-git-send-email-luis.henriques@canonical.com |
---|---|
State | New |
Headers | show |
A lot of change, though looks to be in agreement of the description. Good it can be cherry-picked, too.
On Mon, Mar 09, 2015 at 12:23:07PM +0000, Luis Henriques wrote: > From: Dave Chinner <dchinner@redhat.com> > > Commit e461fcb ("xfs: remote attribute lookups require the value > length") passes the remote attribute length in the xfs_da_args > structure on lookup so that CRC calculations and validity checking > can be performed correctly by related code. This, unfortunately has > the side effect of changing the args->valuelen parameter in cases > where it shouldn't. > > That is, when we replace a remote attribute, the incoming > replacement stores the value and length in args->value and > args->valuelen, but then the lookup which finds the existing remote > attribute overwrites args->valuelen with the length of the remote > attribute being replaced. Hence when we go to create the new > attribute, we create it of the size of the existing remote > attribute, not the size it is supposed to be. When the new attribute > is much smaller than the old attribute, this results in a > transaction overrun and an ASSERT() failure on a debug kernel: > > XFS: Assertion failed: tp->t_blk_res_used <= tp->t_blk_res, file: fs/xfs/xfs_trans.c, line: 331 > > Fix this by keeping the remote attribute value length separate to > the attribute value length in the xfs_da_args structure. The enables > us to pass the length of the remote attribute to be removed without > overwriting the new attribute's length. > > Also, ensure that when we save remote block contexts for a later > rename we zero the original state variables so that we don't confuse > the state of the attribute to be removes with the state of the new > attribute that we just added. [Spotted by Brain Foster.] > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > Signed-off-by: Dave Chinner <david@fromorbit.com> > (cherry picked from commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59) > CVE-2015-0274 > BugLink: http://bugs.launchpad.net/bugs/1429824 > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > fs/xfs/xfs_attr.c | 24 +++++++++++++++++++++++- > fs/xfs/xfs_attr_leaf.c | 21 +++++++++++---------- > fs/xfs/xfs_attr_list.c | 1 + > fs/xfs/xfs_attr_remote.c | 8 +++++--- > fs/xfs/xfs_da_btree.h | 2 ++ > 5 files changed, 42 insertions(+), 14 deletions(-) > > diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c > index b86127072ac3..72c26d52e53f 100644 > --- a/fs/xfs/xfs_attr.c > +++ b/fs/xfs/xfs_attr.c > @@ -212,7 +212,7 @@ xfs_attr_calc_size( > * Out of line attribute, cannot double split, but > * make room for the attribute value itself. > */ > - uint dblocks = XFS_B_TO_FSB(mp, valuelen); > + uint dblocks = xfs_attr3_rmt_blocks(mp, valuelen); > nblks += dblocks; > nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK); > } > @@ -697,11 +697,22 @@ xfs_attr_leaf_addname(xfs_da_args_t *args) > > trace_xfs_attr_leaf_replace(args); > > + /* save the attribute state for later removal*/ > args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */ > args->blkno2 = args->blkno; /* set 2nd entry info*/ > args->index2 = args->index; > args->rmtblkno2 = args->rmtblkno; > args->rmtblkcnt2 = args->rmtblkcnt; > + args->rmtvaluelen2 = args->rmtvaluelen; > + > + /* > + * clear the remote attr state now that it is saved so that the > + * values reflect the state of the attribute we are about to > + * add, not the attribute we just found and will remove later. > + */ > + args->rmtblkno = 0; > + args->rmtblkcnt = 0; > + args->rmtvaluelen = 0; > } > > /* > @@ -793,6 +804,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args) > args->blkno = args->blkno2; > args->rmtblkno = args->rmtblkno2; > args->rmtblkcnt = args->rmtblkcnt2; > + args->rmtvaluelen = args->rmtvaluelen2; > if (args->rmtblkno) { > error = xfs_attr_rmtval_remove(args); > if (error) > @@ -998,13 +1010,22 @@ restart: > > trace_xfs_attr_node_replace(args); > > + /* save the attribute state for later removal*/ > args->op_flags |= XFS_DA_OP_RENAME; /* atomic rename op */ > args->blkno2 = args->blkno; /* set 2nd entry info*/ > args->index2 = args->index; > args->rmtblkno2 = args->rmtblkno; > args->rmtblkcnt2 = args->rmtblkcnt; > + args->rmtvaluelen2 = args->rmtvaluelen; > + > + /* > + * clear the remote attr state now that it is saved so that the > + * values reflect the state of the attribute we are about to > + * add, not the attribute we just found and will remove later. > + */ > args->rmtblkno = 0; > args->rmtblkcnt = 0; > + args->rmtvaluelen = 0; > } > > retval = xfs_attr3_leaf_add(blk->bp, state->args); > @@ -1132,6 +1153,7 @@ restart: > args->blkno = args->blkno2; > args->rmtblkno = args->rmtblkno2; > args->rmtblkcnt = args->rmtblkcnt2; > + args->rmtvaluelen = args->rmtvaluelen2; > if (args->rmtblkno) { > error = xfs_attr_rmtval_remove(args); > if (error) > diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c > index 7b126f46a2f9..6bad3ef656aa 100644 > --- a/fs/xfs/xfs_attr_leaf.c > +++ b/fs/xfs/xfs_attr_leaf.c > @@ -1228,6 +1228,7 @@ xfs_attr3_leaf_add_work( > name_rmt->valueblk = 0; > args->rmtblkno = 1; > args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen); > + args->rmtvaluelen = args->valuelen; > } > xfs_trans_log_buf(args->trans, bp, > XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index), > @@ -2166,11 +2167,11 @@ xfs_attr3_leaf_lookup_int( > if (!xfs_attr_namesp_match(args->flags, entry->flags)) > continue; > args->index = probe; > - args->valuelen = be32_to_cpu(name_rmt->valuelen); > + args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen); > args->rmtblkno = be32_to_cpu(name_rmt->valueblk); > args->rmtblkcnt = xfs_attr3_rmt_blocks( > args->dp->i_mount, > - args->valuelen); > + args->rmtvaluelen); > return XFS_ERROR(EEXIST); > } > } > @@ -2219,19 +2220,19 @@ xfs_attr3_leaf_getvalue( > name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index); > ASSERT(name_rmt->namelen == args->namelen); > ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0); > - valuelen = be32_to_cpu(name_rmt->valuelen); > + args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen); > args->rmtblkno = be32_to_cpu(name_rmt->valueblk); > args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount, > - valuelen); > + args->rmtvaluelen); > if (args->flags & ATTR_KERNOVAL) { > - args->valuelen = valuelen; > + args->valuelen = args->rmtvaluelen; > return 0; > } > - if (args->valuelen < valuelen) { > - args->valuelen = valuelen; > + if (args->valuelen < args->rmtvaluelen) { > + args->valuelen = args->rmtvaluelen; > return XFS_ERROR(ERANGE); > } > - args->valuelen = valuelen; > + args->valuelen = args->rmtvaluelen; > } > return 0; > } > @@ -2518,7 +2519,7 @@ xfs_attr3_leaf_clearflag( > ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0); > name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index); > name_rmt->valueblk = cpu_to_be32(args->rmtblkno); > - name_rmt->valuelen = cpu_to_be32(args->valuelen); > + name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen); > xfs_trans_log_buf(args->trans, bp, > XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt))); > } > @@ -2676,7 +2677,7 @@ xfs_attr3_leaf_flipflags( > ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0); > name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index); > name_rmt->valueblk = cpu_to_be32(args->rmtblkno); > - name_rmt->valuelen = cpu_to_be32(args->valuelen); > + name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen); > xfs_trans_log_buf(args->trans, bp1, > XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt))); > } > diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c > index 2d174b128153..27eba57b87b3 100644 > --- a/fs/xfs/xfs_attr_list.c > +++ b/fs/xfs/xfs_attr_list.c > @@ -447,6 +447,7 @@ xfs_attr3_leaf_list_int( > args.dp = context->dp; > args.whichfork = XFS_ATTR_FORK; > args.valuelen = valuelen; > + args.rmtvaluelen = valuelen; > args.value = kmem_alloc(valuelen, KM_SLEEP | KM_NOFS); > args.rmtblkno = be32_to_cpu(name_rmt->valueblk); > args.rmtblkcnt = xfs_attr3_rmt_blocks( > diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c > index 5549d69ddb45..1b38936acd3e 100644 > --- a/fs/xfs/xfs_attr_remote.c > +++ b/fs/xfs/xfs_attr_remote.c > @@ -340,7 +340,7 @@ xfs_attr_rmtval_get( > struct xfs_buf *bp; > xfs_dablk_t lblkno = args->rmtblkno; > __uint8_t *dst = args->value; > - int valuelen = args->valuelen; > + int valuelen; > int nmap; > int error; > int blkcnt = args->rmtblkcnt; > @@ -350,7 +350,9 @@ xfs_attr_rmtval_get( > trace_xfs_attr_rmtval_get(args); > > ASSERT(!(args->flags & ATTR_KERNOVAL)); > + ASSERT(args->rmtvaluelen == args->valuelen); > > + valuelen = args->rmtvaluelen; > while (valuelen > 0) { > nmap = ATTR_RMTVALUE_MAPSIZE; > error = xfs_bmapi_read(args->dp, (xfs_fileoff_t)lblkno, > @@ -418,7 +420,7 @@ xfs_attr_rmtval_set( > * attributes have headers, we can't just do a straight byte to FSB > * conversion and have to take the header space into account. > */ > - blkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen); > + blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen); > error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff, > XFS_ATTR_FORK); > if (error) > @@ -483,7 +485,7 @@ xfs_attr_rmtval_set( > */ > lblkno = args->rmtblkno; > blkcnt = args->rmtblkcnt; > - valuelen = args->valuelen; > + valuelen = args->rmtvaluelen; > while (valuelen > 0) { > struct xfs_buf *bp; > xfs_daddr_t dblkno; > diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h > index 6e95ea79f5d7..201c6091d26a 100644 > --- a/fs/xfs/xfs_da_btree.h > +++ b/fs/xfs/xfs_da_btree.h > @@ -60,10 +60,12 @@ typedef struct xfs_da_args { > int index; /* index of attr of interest in blk */ > xfs_dablk_t rmtblkno; /* remote attr value starting blkno */ > int rmtblkcnt; /* remote attr value block count */ > + int rmtvaluelen; /* remote attr value length in bytes */ > xfs_dablk_t blkno2; /* blkno of 2nd attr leaf of interest */ > int index2; /* index of 2nd attr in blk */ > xfs_dablk_t rmtblkno2; /* remote attr value starting blkno */ > int rmtblkcnt2; /* remote attr value block count */ > + int rmtvaluelen2; /* remote attr value length in bytes */ > int op_flags; /* operation flags */ > enum xfs_dacmp cmpresult; /* name compare result for lookups */ > } xfs_da_args_t; > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Looks ok
Applied to trusty. -apw
diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c index b86127072ac3..72c26d52e53f 100644 --- a/fs/xfs/xfs_attr.c +++ b/fs/xfs/xfs_attr.c @@ -212,7 +212,7 @@ xfs_attr_calc_size( * Out of line attribute, cannot double split, but * make room for the attribute value itself. */ - uint dblocks = XFS_B_TO_FSB(mp, valuelen); + uint dblocks = xfs_attr3_rmt_blocks(mp, valuelen); nblks += dblocks; nblks += XFS_NEXTENTADD_SPACE_RES(mp, dblocks, XFS_ATTR_FORK); } @@ -697,11 +697,22 @@ xfs_attr_leaf_addname(xfs_da_args_t *args) trace_xfs_attr_leaf_replace(args); + /* save the attribute state for later removal*/ args->op_flags |= XFS_DA_OP_RENAME; /* an atomic rename */ args->blkno2 = args->blkno; /* set 2nd entry info*/ args->index2 = args->index; args->rmtblkno2 = args->rmtblkno; args->rmtblkcnt2 = args->rmtblkcnt; + args->rmtvaluelen2 = args->rmtvaluelen; + + /* + * clear the remote attr state now that it is saved so that the + * values reflect the state of the attribute we are about to + * add, not the attribute we just found and will remove later. + */ + args->rmtblkno = 0; + args->rmtblkcnt = 0; + args->rmtvaluelen = 0; } /* @@ -793,6 +804,7 @@ xfs_attr_leaf_addname(xfs_da_args_t *args) args->blkno = args->blkno2; args->rmtblkno = args->rmtblkno2; args->rmtblkcnt = args->rmtblkcnt2; + args->rmtvaluelen = args->rmtvaluelen2; if (args->rmtblkno) { error = xfs_attr_rmtval_remove(args); if (error) @@ -998,13 +1010,22 @@ restart: trace_xfs_attr_node_replace(args); + /* save the attribute state for later removal*/ args->op_flags |= XFS_DA_OP_RENAME; /* atomic rename op */ args->blkno2 = args->blkno; /* set 2nd entry info*/ args->index2 = args->index; args->rmtblkno2 = args->rmtblkno; args->rmtblkcnt2 = args->rmtblkcnt; + args->rmtvaluelen2 = args->rmtvaluelen; + + /* + * clear the remote attr state now that it is saved so that the + * values reflect the state of the attribute we are about to + * add, not the attribute we just found and will remove later. + */ args->rmtblkno = 0; args->rmtblkcnt = 0; + args->rmtvaluelen = 0; } retval = xfs_attr3_leaf_add(blk->bp, state->args); @@ -1132,6 +1153,7 @@ restart: args->blkno = args->blkno2; args->rmtblkno = args->rmtblkno2; args->rmtblkcnt = args->rmtblkcnt2; + args->rmtvaluelen = args->rmtvaluelen2; if (args->rmtblkno) { error = xfs_attr_rmtval_remove(args); if (error) diff --git a/fs/xfs/xfs_attr_leaf.c b/fs/xfs/xfs_attr_leaf.c index 7b126f46a2f9..6bad3ef656aa 100644 --- a/fs/xfs/xfs_attr_leaf.c +++ b/fs/xfs/xfs_attr_leaf.c @@ -1228,6 +1228,7 @@ xfs_attr3_leaf_add_work( name_rmt->valueblk = 0; args->rmtblkno = 1; args->rmtblkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen); + args->rmtvaluelen = args->valuelen; } xfs_trans_log_buf(args->trans, bp, XFS_DA_LOGRANGE(leaf, xfs_attr3_leaf_name(leaf, args->index), @@ -2166,11 +2167,11 @@ xfs_attr3_leaf_lookup_int( if (!xfs_attr_namesp_match(args->flags, entry->flags)) continue; args->index = probe; - args->valuelen = be32_to_cpu(name_rmt->valuelen); + args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen); args->rmtblkno = be32_to_cpu(name_rmt->valueblk); args->rmtblkcnt = xfs_attr3_rmt_blocks( args->dp->i_mount, - args->valuelen); + args->rmtvaluelen); return XFS_ERROR(EEXIST); } } @@ -2219,19 +2220,19 @@ xfs_attr3_leaf_getvalue( name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index); ASSERT(name_rmt->namelen == args->namelen); ASSERT(memcmp(args->name, name_rmt->name, args->namelen) == 0); - valuelen = be32_to_cpu(name_rmt->valuelen); + args->rmtvaluelen = be32_to_cpu(name_rmt->valuelen); args->rmtblkno = be32_to_cpu(name_rmt->valueblk); args->rmtblkcnt = xfs_attr3_rmt_blocks(args->dp->i_mount, - valuelen); + args->rmtvaluelen); if (args->flags & ATTR_KERNOVAL) { - args->valuelen = valuelen; + args->valuelen = args->rmtvaluelen; return 0; } - if (args->valuelen < valuelen) { - args->valuelen = valuelen; + if (args->valuelen < args->rmtvaluelen) { + args->valuelen = args->rmtvaluelen; return XFS_ERROR(ERANGE); } - args->valuelen = valuelen; + args->valuelen = args->rmtvaluelen; } return 0; } @@ -2518,7 +2519,7 @@ xfs_attr3_leaf_clearflag( ASSERT((entry->flags & XFS_ATTR_LOCAL) == 0); name_rmt = xfs_attr3_leaf_name_remote(leaf, args->index); name_rmt->valueblk = cpu_to_be32(args->rmtblkno); - name_rmt->valuelen = cpu_to_be32(args->valuelen); + name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen); xfs_trans_log_buf(args->trans, bp, XFS_DA_LOGRANGE(leaf, name_rmt, sizeof(*name_rmt))); } @@ -2676,7 +2677,7 @@ xfs_attr3_leaf_flipflags( ASSERT((entry1->flags & XFS_ATTR_LOCAL) == 0); name_rmt = xfs_attr3_leaf_name_remote(leaf1, args->index); name_rmt->valueblk = cpu_to_be32(args->rmtblkno); - name_rmt->valuelen = cpu_to_be32(args->valuelen); + name_rmt->valuelen = cpu_to_be32(args->rmtvaluelen); xfs_trans_log_buf(args->trans, bp1, XFS_DA_LOGRANGE(leaf1, name_rmt, sizeof(*name_rmt))); } diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c index 2d174b128153..27eba57b87b3 100644 --- a/fs/xfs/xfs_attr_list.c +++ b/fs/xfs/xfs_attr_list.c @@ -447,6 +447,7 @@ xfs_attr3_leaf_list_int( args.dp = context->dp; args.whichfork = XFS_ATTR_FORK; args.valuelen = valuelen; + args.rmtvaluelen = valuelen; args.value = kmem_alloc(valuelen, KM_SLEEP | KM_NOFS); args.rmtblkno = be32_to_cpu(name_rmt->valueblk); args.rmtblkcnt = xfs_attr3_rmt_blocks( diff --git a/fs/xfs/xfs_attr_remote.c b/fs/xfs/xfs_attr_remote.c index 5549d69ddb45..1b38936acd3e 100644 --- a/fs/xfs/xfs_attr_remote.c +++ b/fs/xfs/xfs_attr_remote.c @@ -340,7 +340,7 @@ xfs_attr_rmtval_get( struct xfs_buf *bp; xfs_dablk_t lblkno = args->rmtblkno; __uint8_t *dst = args->value; - int valuelen = args->valuelen; + int valuelen; int nmap; int error; int blkcnt = args->rmtblkcnt; @@ -350,7 +350,9 @@ xfs_attr_rmtval_get( trace_xfs_attr_rmtval_get(args); ASSERT(!(args->flags & ATTR_KERNOVAL)); + ASSERT(args->rmtvaluelen == args->valuelen); + valuelen = args->rmtvaluelen; while (valuelen > 0) { nmap = ATTR_RMTVALUE_MAPSIZE; error = xfs_bmapi_read(args->dp, (xfs_fileoff_t)lblkno, @@ -418,7 +420,7 @@ xfs_attr_rmtval_set( * attributes have headers, we can't just do a straight byte to FSB * conversion and have to take the header space into account. */ - blkcnt = xfs_attr3_rmt_blocks(mp, args->valuelen); + blkcnt = xfs_attr3_rmt_blocks(mp, args->rmtvaluelen); error = xfs_bmap_first_unused(args->trans, args->dp, blkcnt, &lfileoff, XFS_ATTR_FORK); if (error) @@ -483,7 +485,7 @@ xfs_attr_rmtval_set( */ lblkno = args->rmtblkno; blkcnt = args->rmtblkcnt; - valuelen = args->valuelen; + valuelen = args->rmtvaluelen; while (valuelen > 0) { struct xfs_buf *bp; xfs_daddr_t dblkno; diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h index 6e95ea79f5d7..201c6091d26a 100644 --- a/fs/xfs/xfs_da_btree.h +++ b/fs/xfs/xfs_da_btree.h @@ -60,10 +60,12 @@ typedef struct xfs_da_args { int index; /* index of attr of interest in blk */ xfs_dablk_t rmtblkno; /* remote attr value starting blkno */ int rmtblkcnt; /* remote attr value block count */ + int rmtvaluelen; /* remote attr value length in bytes */ xfs_dablk_t blkno2; /* blkno of 2nd attr leaf of interest */ int index2; /* index of 2nd attr in blk */ xfs_dablk_t rmtblkno2; /* remote attr value starting blkno */ int rmtblkcnt2; /* remote attr value block count */ + int rmtvaluelen2; /* remote attr value length in bytes */ int op_flags; /* operation flags */ enum xfs_dacmp cmpresult; /* name compare result for lookups */ } xfs_da_args_t;