Re: [PATCH] xfs: remote attribute overwrite causes transaction overrun

[Sheng Yong <shengyong1@huawei.com>]

Hi, Greg

On 5/3/2015 2:36 AM, Greg KH wrote:
> On Wed, Apr 29, 2015 at 05:05:17PM +0800, Sheng Yong wrote:
>> Hi, Dave,
>> Thanks for your review.
>>
>> On 4/29/2015 3:43 PM, Dave Chinner wrote:
>>> On Wed, Apr 29, 2015 at 01:55:25AM +0000, Sheng Yong wrote:
>>>> commit 8275cdd0e7ac550dcce2b3ef6d2fb3b808c1ae59 upstream.
>>>>
>>>> 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>
>>>> [shengyong: backport to 3.10
>>>>  - Addresse: CVE-2015-0274
>>>>  - adjust context
>>>>  - fs/xfs/xfs_attr_list.c comes from fs/xfs/xfs_attr.c and
>>>>    fs/xfs/xfs_attr_leaf.c in linux 3.12]
>>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>>
>>> You are backporting to 3.10?
>>>
>>> Check when Commit e461fcb ("xfs: remote attribute lookups require
>>> the value length") was introduced:
>> You are right, commit e461fcb is not merged into 3.10, but as I mentioned in
>> the cover letter, commit 7ae077802 cherry-picked from e461fcb, and the cherry-pick
>> one is merged into 3.10 :-)
>>
>> linux-stable$ git log --oneline 7ae077802 -n 1
>> 7ae0778 xfs: remote attribute lookups require the value length
>>
>> linux-stable$ git describe --contains 7ae077802
>> v3.10-rc3~5^2
> 
> I don't understand this at all, what do you mean?
> 
> confused,
CVE-2015-0274 is caused by commit e461fcb ("xfs: remote attribute lookups require
the value length"), which was introduced in 3.11. It should have had nothing to
do with 3.10-stable. However, when we checked 3.10, we found that this commit was
check-picked from (maybe) the xfs tree. The patch ("xfs: remote attribute lookups
require the value length") was also included in 3.10, and its commit is 7ae077802.
So 3.10-stable is affected by the CVE.

thanks,
Sheng
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
>