Capsicumise dd

[kaktus]

Summary

This patch puts dd in capsicum based sandbox.

Sponsored by: Mysterious Code Ltd.

Test Plan

Tested with make test and by hand with various options like seek and skip.

Tape _untested_ but it should work as all ioctls related to tape are permitted.

[allanjude]

freebsd/bin/dd/dd.c

179 ↗

(On Diff #22267)

it might make more sense to do this on like 160, where we set CAP_WRITE etc

[kaktus]

freebsd/bin/dd/dd.c

179 ↗

(On Diff #22267)

But I want to _clear_ the flag if the file is open write only.

[allanjude]

freebsd/bin/dd/dd.c

179 ↗

(On Diff #22267)

I misunderstood what was happening, sorry.

This appears correct.

[kaktus]

Regenerate patch.

[kaktus]

Regenerate, this time with -U999999.

[oshogbo]

bin/dd/dd.c

100 ↗

(On Diff #22279)

We should check errno and cap_enter in one line.
Basically you should not warn if Capsicum is unavailable, user decided to compile kernel without CAPSICUM so we should not inform go about that on every step.
Some architectures even don't have Capsicum enable by default.

140 ↗

(On Diff #22279)

Style.
{ MTIOCTOP, FIODTYPE }

192 ↗

(On Diff #22279)

Style. To big indent, should be tab and 4 spaces.

196 ↗

(On Diff #22279)

Here you don't need to check errno.

[oshogbo]

bin/dd/dd.c

183 ↗

(On Diff #22279)

Here you also don't need to check errno.

[emaste]

bin/dd/dd.c

196 ↗

(On Diff #22279)

To clarify, the caph_ helpers handle the errno checking internally.

[oshogbo]

bin/dd/dd.c

98 ↗

(On Diff #22279)

Sorry for being PITA, but do we need that?
Which function requires time zones?

[kaktus]

Updated diff after requested changes.

[kaktus]

bin/dd/dd.c

140 ↗

(On Diff #22279)

Maybe, as I don't see this rule defined _clearly_ in style(9).

[robak]

@oshogbo @allanjude It works for me, when I test it, so if I can get approval from someone with src commit bit, I am happy to commit it.

[emaste]

bin/dd/dd.c

140 ↗

(On Diff #22279)

Some of style(9) is by example, not explicit - this is probably the closest even though it's for something else:

Enumeration values are all uppercase.

enum enumtype { ONE, TWO } et;

192 ↗

(On Diff #22279)

Line 188 should be:
<tab><space><space><space><space>errno != ENOSYS
looking at the raw diff it has two tabs and four spaces in the most recent upload

[kaktus]

Fix indentation.

[emaste]

I think this looks good, and have asked @oshogbo if he can check again.

Also, let me say thank you for your interest and effort in applying Capsicum and submitting these patches.

[oshogbo]

Good work :)
Please just fix the error messages.

bin/dd/dd.c

146 ↗

(On Diff #22334)

One more thing.
You are inconsistent in error messages.
Everywhere in file we have sentence start from small later ;(

[kaktus]

• be more consistent with lowercase in err().

[kaktus]

Make it compile on older releases as dd is part of bootstrap and pre 12-C don't have capsicum_helpers.h installed.

Tested on 10.3-RELEASE i386 building head r308913.

[robak]

Reopening due to dd/build toolchain issues.

[oshogbo]

Hi,

Thanks for you work, but I like more your previus patch :)
So I would like to fix the problem with that patch in a little different meaner basically install libcapsicum.
I will do that today, and replay your patch.

Thank you one more time!

[imp]

I think we should have a fake libcapiscim that's all defeined as no-op success functions and add that to libegacy. We don't need to install the full libcapsicim and there's ordering issues trying to do so.

However, we don't need it for dd. I have patches in the works that remove the need to make it a bootstrap tool. I'll post a review shortly.

So the X is for 'install libcapsicim' not for this patch as it is once I get the Makefile stuff sorted.

[imp]

See https://reviews.freebsd.org/D8605 for one possible resolution

[kaktus]

As I see libcapsicum is header only https://svnweb.freebsd.org/base/head/lib/libcapsicum/Makefile?revision=306726&view=markup.
The breakage was obviously caused by lack of that header in installed system older than 12-C.

So either we 1) install that lib / header 2) remove dd from bootstrap 3) use the current patch (I agree that this is the less favourable option).

I'll leave it up to you guys to decide :-)

[kaktus]

Since dd was removed from bootstrap in 309412, can this be recommitted?

[imp]

In D8543#180755, @pawel.biernacki-gmail.com wrote:

Since dd was removed from bootstrap in 309412, can this be recommitted?

If you can bootstrap with an old dd binary, sure. Since we no longer build dd as a bootstrap tool, I anticipate that there will be smooth sailing. But test from 10.3R and 11.0R on amd64 just to make sure, along with at least one universe.

That's rather the whole reason I did this. :)

[imp]

Should have ticked 'accept'

[kaktus]

Update diff with one that survived building head@r309672:

1. buildword && buildkernel on FreeBSD 10.3-R i386
2. universe on FreeBSD 11.0-R amd64

[robak]

Guys, are we happy with the state of things? I am keen to commit it, given relevant approval is provided.

[kaktus]

Fix regression when stdin/out/err fds are are overridden by shell.
Found by Kyua tests.

bin/dd/dd_test:io -> passed [0.038s]
bin/dd/dd_test:length -> passed [0.022s]
bin/dd/dd_test:seek -> passed [0.060s]

[robak]

@ngie Thoughts? I'd like to commit this and move on to other things, so review/approval is highly appreciated.

[ngie]

The changes proposed seem ok, in so long as it addresses the regression I reported on svn-src-all@.