On Mon, Apr 2, 2018 at 10:45 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Fri, Mar 30, 2018 at 12:12:18PM +0300, Amir Goldstein wrote:
>> 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.
>
> Ok.
>
>>
>>
>> >> + } 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.
>
> Ok, So I could make it like the case of directory. That is for
> metacopy files bless origin only if it was verified and modify
> above condiiton as follows.
>
> if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> origin = stack[0].dentry;
>
> That way for a metcopy dentry we will search for index only if
> origin was verified.
>
Correct.
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
