On Fri, Mar 30, 2018 at 8:49 AM, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> On Thu, Mar 29, 2018 at 10:38 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>> This patch modifies ovl_lookup() and friends to lookup metacopy dentries.
>> It also allows for presence of metacopy dentries in lower layer.
>>
>> During lookup, check for presence of OVL_XATTR_METACOPY and if not present,
>> set OVL_UPPERDATA bit in flags.
>>
>> We don't support metacopy feature with nfs_export. So in nfs_export code,
>> we set OVL_UPPERDATA flag set unconditionally if upper inode exists.
>>
>> Do not follow metacopy origin if we find a metacopy only inode and metacopy
>> feature is not enabled for that mount. Like redirect, this can have security
>> implications where an attacker could hand craft upper and try to gain
>> access to file on lower which it should not have to begin with.
>>
>> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
>> ---
[...]
>> @@ -917,16 +929,35 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> * When "verify_lower" feature is enabled, do not merge with a
>> * lower dir that does not match a stored origin xattr. In any
>> * case, only verified origin is used for index lookup.
>> + *
>> + * For non-dir dentry, make sure dentry found by lookup
>> + * matches the origin stored in upper. Otherwise its an
>> + * error.
>> */
>> - if (upperdentry && !ctr && ovl_verify_lower(dentry->d_sb)) {
>> + if (upperdentry && !ctr &&
>> + ((d.is_dir && ovl_verify_lower(dentry->d_sb)) ||
>> + (!d.is_dir && origin_path))) {
>> err = ovl_verify_origin(upperdentry, this, false);
>> if (err) {
>> dput(this);
>> - break;
>> + if (d.is_dir)
>> + break;
>> + goto out_put;
>> }
>> -
>> /* Bless lower dir as verified origin */
>> - origin = this;
>> + if (d.is_dir)
>> + origin = this;
>
> It's ok to bless verified non-dir as well.
> It is going to be blesses anyway, just above index lookup if ctr > 0.
>
>> + }
>> +
>> + if (d.metacopy)
>> + metacopy = true;
>> + /*
>> + * Do not store intermediate metacopy dentries in chain,
>> + * except top most lower metacopy dentry
>> + */
>> + if (d.metacopy && ctr) {
>> + dput(this);
>> + continue;
>> }
>>
>> stack[ctr].dentry = this;
>> @@ -960,6 +991,34 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>> }
>> }
>>
>> + if (metacopy) {
>> + BUG_ON(d.is_dir);
>
> Yeh, I think that is really a bug, because you need to detect
> the case of dir in lower layer under metacopy in upper layer
> and do something about it.
>
>> + /*
>> + * Found a metacopy dentry but did not find corresponding
>> + * data dentry
>> + */
>> + if (d.metacopy) {
>> + err = -ESTALE;
>> + goto out_put;
>> + }
>> +
>> + err = -EPERM;
>> + if (!ofs->config.metacopy) {
>> + pr_warn_ratelimited("overlay: refusing to follow"
>> + " metacopy origin for (%pd2)\n",
>> + dentry);
>> + goto out_put;
>> + }
in (!origin_path) lower was followed by name/redirect and not
verified by origin, we should not lookup index of non-dir below.
Right now non-dir index entries assume that origin xattr exists
and matches the entry name. We may be able to relax that in
the future using non-dir redirect, but we are not there yet.
>> + } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
>> + if (WARN_ON(stack != NULL)) {
>> + err = -EIO;
>> + goto out_put;
>> + }
>> + stack = origin_path;
>> + ctr = 1;
>> + origin_path = NULL;
>> + }
>> +
>> /*
>> * Lookup index by lower inode and verify it matches upper inode.
>> * We only trust dir index if we verified that lower dir matches
* origin, otherwise dir index entries may be inconsistent and we
* ignore them. Always lookup index of non-dir and non-upper.
*/
if (ctr && (!upperdentry || !d.is_dir))
origin = stack[0].dentry;
So this condition needs to be fixed.
Thanks,
Amir.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
