Re: [pve-devel] [PATCH v2 storage 3/5] btrfs: add 'btrfs' import/export format

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Wed, Jun 23, 2021 at 02:14:48PM +0200, Fabian Grünbichler wrote:
> On June 22, 2021 2:18 pm, Wolfgang Bumiller wrote:
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

(...)

> > +sub volume_export {
> > +    my (
> > +	$class,
> > +	$scfg,
> > +	$storeid,
> > +	$fh,
> > +	$volname,
> > +	$format,
> > +	$snapshot,
> > +	$base_snapshot,
> > +	$with_snapshots,
> > +    ) = @_;
> > +
> > +    if ($format ne 'btrfs') {
> > +	return PVE::Storage::Plugin::volume_export(@_);
> > +    }
> > +
> > +    die "format 'btrfs' only works on snapshots\n"
> > +	if !defined $snapshot;
> > +
> > +    die "'btrfs' format in incremental mode requires snapshots to be listed explicitly\n"
> > +	if defined($base_snapshot) && $with_snapshots && ref($with_snapshots) ne 'ARRAY';
> > +
> > +    my $volume_format = ($class->parse_volname($volname))[6];
> > +
> > +    die "btrfs-sending volumes of type $volume_format ('$volname') is not supported\n"
> > +	if $volume_format ne 'raw' && $volume_format ne 'subvol';
> 
> the three checks here are the same as in volume_export, maybe they could 
> go into a single export-helper?

we get nicer errors this way, though (and more easily placed in the code
when a user runs into them, since we don't use Carp ;-) )

(...)

> > +
> > +	# Now go through the remaining snapshots (if any)
> > +	foreach my $snap (@snapshots) {
> > +	    $class->btrfs_cmd(['property', 'set', "$tmppath/$diskname\@$snap", 'ro', 'false']);
> > +	    PVE::Tools::renameat2(
> > +		-1,
> > +		"$tmppath/$diskname\@$snap",
> > +		-1,
> > +		"$destination\@$snap",
> > +		&PVE::Tools::RENAME_NOREPLACE,
> > +	    ) or die "failed to move received snapshot '$tmppath/$diskname\@$snap'"
> > +		. " into place at '$destination\@$snap' - $!\n";
> > +	    eval { $class->btrfs_cmd(['property', 'set', "$destination\@$snap", 'ro', 'true']) };
> > +	    warn "failed to make $destination\@$snap read-only - $!\n" if $@;
> > +	}
> > +    };
> > +    my $err = $@;
> > +
> > +    eval {
> > +	# Cleanup all the received snapshots we did not move into place, so we can remove the temp
> > +	# directory.
> > +	if ($dh) {
> > +	    $dh->rewind;
> > +	    while (defined(my $entry = $dh->read)) {
> > +		next if $entry eq '.' || $entry eq '..';
> > +		eval { $class->btrfs_cmd(['subvolume', 'delete', '--', "$tmppath/$entry"]) };
> > +		warn $@ if $@;
> > +	    }
> > +	    $dh->close; undef $dh;
> > +	}
> > +	if (!rmdir($tmppath)) {
> > +	    warn "failed to remove temporary directory '$tmppath' - $!\n"
> > +	}
> > +    };
> > +    warn $@ if $@;
> > +    if ($err) {
> > +	# clean up if the directory ended up being empty after an error
> > +	rmdir($tmppath);
> > +	die $err;
> > +    }
> 
> I am a bit wary about this (also w.r.t. future btrfs features) - a 
> privileged container can do quite a lot of stuff with btrfs
> 
> - create snapshots from within container
> - set default subvolume
> - set properties
> - ... (the above is what I found in a few minutes without much BTRFS 
>   experience)
> 
> unprivileged containers can do the following
> - create new subvols
> - create new snapshots
> - set ro property
> 
> (same caveat of not looking too much at what else is possible, lots of 
> stuff does not work)
> 
> how do we guarantee that the send stream is not affected in a dangerous 
> fashion by an attacker inside the container?

Shouldn't be dangerous sine it's not recursive anyway.

However, we could just by default disable the corresponding ioctls via
seccomp for unprivileged containers...