On Sun, Mar 31, 2019 at 8:41 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Sat, Mar 30, 2019 at 10:45 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > > > On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > This nasty little syzbot repro: > > > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000 > > > > > > Creates overlay mounts where the same directory is both in upper > > > and lower layers. Simplified example: > > > > > > mkdir foo work > > > mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work" > > > > Shouldn't the mount fail in this case? > > > > Sure, but that is independent of this patch, because renaming > between lower layers after mount can get you to the same > situation and that is harder to prevent. > > > Does it make any sense to allow overlapping layers? > > > > If doing the check the dumb way, the number of d_ancestor() calls > > would increase quadratically with the number of layers, but I think > > it's possible to do it in linear time if necessary. > > As I wrote to Vivek, we can do that, but it's only going to > benefit users that make an innocent mistake (what's the odds of that?). > An adversary could easily work around the mount check by moving > lower dirs after mount. > What do you guys thing of this idea: On mount we "poison" overlay inode cache with canary overlay inode for each layer root (including upper and work). The canary inode has the layer root inode as i_private, but both ovl_inode_lower() and ovl_inode_upper() NULL, so it will always fail ovl_verify_inode(). Then in ovl_lookup_single(), we do a sanity ovl_lookup_inode(this->d_inode). If it returns -ESTALE, we fail the lookup. This should immune overlayfs from moving layers root into other layers after mount. During mount, after setting up all canary inodes, we can walk back ancestors of all layers roots and use ovl_lookup_inode() on real ancestors to check for bad setup. A simple optimization for docker and similar container managers - if all layers roots have the same parent or same grandparent (e.g. /var/lib/docker/overlay), we can skip the ancestors walk on mount, but still keep the lookup sanity checks for post mount tampering protection. Thoughts? Amir.
