Add reserve block percentage option for formatting

[rainemak]

Add reserve block percentage option for formatting. When formatting large memory cards with ext4 it'd be good to have possibility to adjust reserve block percentage.

Currently options passed to build_command [1] can only include option_no_discard coming from udiskslinuxfsinfo.h that is not applicable for all filesystem types (when "no-discard" is true). Reserved block percentage could be similar to no disard in sense that it is applicable for ext2/ext3/ext4.

[1] https://github.com/storaged-project/udisks/blob/master/src/udiskslinuxblock.c#L3296

[vpodzime]

I will take a look at this over the weekend.

[vpodzime]

See also #584

[vpodzime]

I think it makes sense to add a new mkfs.extra_opts option to the UDisks2.Block.Format() method that would allow passing arbitrary extra options for the mkfs command. Adding them one by one especially when some are FS-specific doesn't really scale.

[tbzatek]

So we have multiple requests for various mkfs options:

• reserved block percentage
• casefolding / case-insensitive / encoding (https://gitlab.gnome.org/GNOME/gnome-disk-utility/-/issues/212)
• cluster size / block size (https://gitlab.gnome.org/GNOME/gnome-disk-utility/-/issues/209)
• FAT revision (FAT12/FAT16/FAT32) (#1010)
• XFS reflinks (#722)

[bertogg]

Hello, does anyone know the status of this? I would be interested in adding a way to pass arbitrary options to mkfs and if no one else has the time to do it I can probably give it a go myself.

[vpodzime]

There is already https://github.com/storaged-project/udisks/issues/722

[bertogg]

But #722 was closed as a duplicate of this one, my question is whether someone is working or planning to work on it. Thanks!

[vpodzime]

But #722 was closed as a duplicate of this one, my question is whether someone is working or planning to work on it. Thanks!

Oh, sorry! I thought it was a PR! :sweat_smile:

[tbzatek]

So AFAIR the only thing that held us back was a question of security.

API wise it would be just another option to the org.freedesktop.UDisks2.Block.Format() method call and passed down to bd_fs_mkfs(). The extra arguments are just appended after the default set of mkfs options as defined in libblockdev. We've agreed not to do any extra validation. It is a caller responsibility that the supplied arguments will succeed - especially forward-looking to any future corresponding mkfs tool changes and also changes to the libblockdev fs plugin code. So no guarantees that the client's code will always work. This must be explicitly noted in the API docs.

Security-wise the options are passed as argv and should be pretty immune to any shell (un)escaping. However imagine that you pass another block device as an extra option. It will take privilege as the block object in question is appended last (or not, however you never know how the mkfs tool actually works - let's assume this is potentially dangerous). Now the Format() method call typically requires root privileges except of the org.freedesktop.udisks2.modify-device polkit action that defines <allow_active>yes</allow_active> in case of block devices that have been setup by user (e.g. loop, luks). And now you have an attack vector if I understand all of this correctly.

Thoughts? Whitelist?

[tbzatek]

I guess this can be guarded with a separate polkit rule always requiring root auth when the extra option appears. Hope I haven't missed any other possible security issue.

Out of curiosity, what options are you planning to supply? We still have the luxury to break the libblockdev 3.0 filesystem API, at least for another couple of weeks.

[bertogg]

Hi,

you're right that passing arbitrary options to the mkfs command line can be a security problem, not only because the caller can "replace" the original block device with another one, but also because some options in some mkfs programs can write to other files or devices specified by the caller. I had a quick look and I saw at least these:

• mkfs.f2fs -c /dev/XXX /dev/YYY wipes the contents of /dev/XXX
• mkfs.xfs -l /dev/XXX /dev/YYY formats /dev/XXX as an external log file.

So one possibility is to let the user pass any arbitrary option but require root authentication, as you say.

Another possible approach is to have a list of the options that are safe and allow those without any additional requirement. Some options don't take any parameter (mkfs.btrfs --nodiscard) and some take one (can be a comma-separated list like with mkfs.ext4 -O feat1,feat2), so it should not be too hard to identify what options the caller is trying to set and decide if they're allowed or not.

In practice I don't think we need to be completely exhaustive and we can allow only the most common ones, from the list that you linked earlier in this thread: cluster size, features, encoding, ...

About your last question: some of the options I would like to set are reserved blocks, casefolding and encryption.

[bertogg]

Hi, I wrote a patch that shows one possible implementation of this feature: https://github.com/bertogg/udisks/commit/f266550dc453fa35477ffd1057a2a715e52de874

It is simply a prototype, lacks documentation and is incomplete, but it is enough to get the idea.

If you think that this is a reasonable approach I can continue working on it, but if you have other suggestions we can also discuss them.

[tbzatek]

Thanks for working on this, I'm busy with high-priority work on other projects right now.

I think whitelist is not a maintainable way forward from long-term perspective. (and I was hoping to get more feedback on this from @vojtechtrefny or @vpodzime). Either way it should live in libblockdev. But for the moment I'd suggest to just go without it.

Instead, it's important to properly guard this by a polkit rule.

https://github.com/storaged-project/udisks/blob/6991f77e7c51354a4146216968c17a480f9f9aee/src/udiskslinuxblock.c#L2893-L2906

I.e. udisks_daemon_util_setup_by_user() needs to be disregarded when extra options are supplied and either org.freedesktop.udisks2.modify-device-system or org.freedesktop.udisks2.modify-device-other-seat is always used, avoiding the use of org.freedesktop.udisks2.modify-device which defines <allow_active>yes</allow_active>.

It's important to bear in mind that the respective mkfs tool arguments supplied by libblockdev may vary between releases and quirks may be added for future mkfs tool versions (as it happened in the past several times). There are no guarantees of stability in this regard on libblockdev side and there can't be any guarantees on udisks side either. I.e. with future releases it may happen that the extra options supplied by the client may result in operation failure due to changed options in the layers below. This is one of the notes that need to be added in the docs. It is also not clearly defined at which position exactly the extra options will get appened in the resulting mkfs commandline args string.

[bertogg]

Thanks for the feedback, I think your concerns are perfectly reasonable.

So I'll rephrase to confirm that I got everything correctly:

1. Allow passing any arbitrary set of options, and make the user responsible for what happens.

2. Guard this feature by a polkit rule.

3. Since there are no whitelists libblockdev should need little or no changes, UDisks would pass the extra options to bd_fs_mkfs() and that would be it.

(plus tests, docs, etc.)

[tbzatek]

Correct, no changes required on libblockdev side at this point. You may try experimenting with options in the tests that conflict with the default options provided by libblockdev... to see if the error handling in udisks works as expected.