| Message ID | 03ac0ffd82bc1edc3a9baa68d1125f5e8cad93fd.1686565330.git.alexl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ovl: Add support for fs-verity checking of lowerdata | expand |
On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > +fs-verity support > +---------------------- > + > +When metadata copy up is used for a file, then the xattr > +"trusted.overlay.verity" may be set on the metacopy file. This > +specifies the expected fs-verity digest of the lowerdata file. This > +may then be used to verify the content of the source file at the time > +the file is opened. During metacopy copy up overlayfs can also set > +this xattr. > + > +This is controlled by the "verity" mount option, which supports > +these values: > + > +- "off": > + The verity xattr is never used. This is the default if verity > + option is not specified. > +- "on": > + Whenever a metacopy files specifies an expected digest, the > + corresponding data file must match the specified digest. > + When generating a metacopy file the verity xattr will be set > + from the source file fs-verity digest (if it has one). > +- "require": > + Same as "on", but additionally all metacopy files must specify a > + verity xattr. This means metadata copy up will only be used if > + the data file has fs-verity enabled, otherwise a full copy-up is > + used. It looks like my request for improved documentation was not taken, which is unfortunate and makes this patchset difficult to review. - Eric
On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > > +fs-verity support > > +---------------------- > > + > > +When metadata copy up is used for a file, then the xattr > > +"trusted.overlay.verity" may be set on the metacopy file. This > > +specifies the expected fs-verity digest of the lowerdata file. This > > +may then be used to verify the content of the source file at the time > > +the file is opened. During metacopy copy up overlayfs can also set > > +this xattr. > > + > > +This is controlled by the "verity" mount option, which supports > > +these values: > > + > > +- "off": > > + The verity xattr is never used. This is the default if verity > > + option is not specified. > > +- "on": > > + Whenever a metacopy files specifies an expected digest, the > > + corresponding data file must match the specified digest. > > + When generating a metacopy file the verity xattr will be set > > + from the source file fs-verity digest (if it has one). > > +- "require": > > + Same as "on", but additionally all metacopy files must specify a > > + verity xattr. This means metadata copy up will only be used if > > + the data file has fs-verity enabled, otherwise a full copy-up is > > + used. > > It looks like my request for improved documentation was not taken, which is > unfortunate and makes this patchset difficult to review. > Which one? IIRC, you had two requests. One very broad to get the overlayfs.rst document up-to-date: [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/ But I assume you mean the specific request about this sentence: [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/ I must say, I re-read this paragraph and the email thread and I don't understand what it is that you are asking for. I am not trying to dismiss your request, I am trying to understand. I understand that overlayfs.rst is not a high standard spec and that "metacopy files" is not well defined and I am not saying that we cannot improve the documentation. But since you already spent the extra time understanding the inaccuracies of overlayfs.rst during v2 review and we already tried to explain the details of metacopy in v2 review, please elaborate what information is actually missing that made the review of v3 harder for you, so that we can try to improve those parts. Let me try to tell this story in a different way Imagine a feature: send/recv email attachments by reference instead of attaching files, drop them in a network share and send/recv a URL to that share with optional verity sig. verity=off: do not include verity sig when sending emails do not verify verity sig when receiving emails verity=on: include verity sig when sending emails (if available) verify verity sig if it is included in received email verity=require: same as verity=on, but if verity sig is not included in received email, do not follow URL. if verity sig not available when sending, attach the file itself instead of the URL - that is what "otherwise a full copy-up" means in the context above To me this sounds exactly like the documentation above. Is this story clear? If it is, what in the doc above is missing to make it also clear? Thanks, Amir.
On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote: > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > > > +fs-verity support > > > +---------------------- > > > + > > > +When metadata copy up is used for a file, then the xattr > > > +"trusted.overlay.verity" may be set on the metacopy file. This > > > +specifies the expected fs-verity digest of the lowerdata file. This > > > +may then be used to verify the content of the source file at the time > > > +the file is opened. During metacopy copy up overlayfs can also set > > > +this xattr. > > > + > > > +This is controlled by the "verity" mount option, which supports > > > +these values: > > > + > > > +- "off": > > > + The verity xattr is never used. This is the default if verity > > > + option is not specified. > > > +- "on": > > > + Whenever a metacopy files specifies an expected digest, the > > > + corresponding data file must match the specified digest. > > > + When generating a metacopy file the verity xattr will be set > > > + from the source file fs-verity digest (if it has one). > > > +- "require": > > > + Same as "on", but additionally all metacopy files must specify a > > > + verity xattr. This means metadata copy up will only be used if > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > + used. > > > > It looks like my request for improved documentation was not taken, which is > > unfortunate and makes this patchset difficult to review. > > > > Which one? > IIRC, you had two requests. > One very broad to get the overlayfs.rst document up-to-date: > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/ That isn't an accurate summary of what I said. I actually pointed out two specific things that are confusing specifically in the context of this feature. > But I assume you mean the specific request about this sentence: > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/ And that was a third specific thing. I got a detailed response back (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com), which was helpful. Unfortunately, the information in that response hasn't yet found its way into the documentation that is being proposed. In general the proposed documentation reads like the audience is overlayfs developers. It doesn't describe the motivation for the feature or how to use it in each of the two use cases. Maybe that is intended, but it's not what I had expected to see. Side note: the use of the passive voice, e.g. "the xattr may be set" and "the xattr may then be used to verify", should be avoided since it makes it unclear who/what is doing these actions. - Eric
On Tue, Jun 13, 2023 at 8:37 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote: > > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > > > > +fs-verity support > > > > +---------------------- > > > > + > > > > +When metadata copy up is used for a file, then the xattr > > > > +"trusted.overlay.verity" may be set on the metacopy file. This > > > > +specifies the expected fs-verity digest of the lowerdata file. This > > > > +may then be used to verify the content of the source file at the time > > > > +the file is opened. During metacopy copy up overlayfs can also set > > > > +this xattr. > > > > + > > > > +This is controlled by the "verity" mount option, which supports > > > > +these values: > > > > + > > > > +- "off": > > > > + The verity xattr is never used. This is the default if verity > > > > + option is not specified. > > > > +- "on": > > > > + Whenever a metacopy files specifies an expected digest, the > > > > + corresponding data file must match the specified digest. > > > > + When generating a metacopy file the verity xattr will be set > > > > + from the source file fs-verity digest (if it has one). > > > > +- "require": > > > > + Same as "on", but additionally all metacopy files must specify a > > > > + verity xattr. This means metadata copy up will only be used if > > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > > + used. > > > > > > It looks like my request for improved documentation was not taken, which is > > > unfortunate and makes this patchset difficult to review. > > > > > > > Which one? > > IIRC, you had two requests. > > One very broad to get the overlayfs.rst document up-to-date: > > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/ > > That isn't an accurate summary of what I said. I actually pointed out two > specific things that are confusing specifically in the context of this feature. > > > But I assume you mean the specific request about this sentence: > > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/ > > And that was a third specific thing. I got a detailed response back > (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com), > which was helpful. Unfortunately, the information in that response hasn't yet > found its way into the documentation that is being proposed. > > In general the proposed documentation reads like the audience is overlayfs > developers. It doesn't describe the motivation for the feature or how to use it > in each of the two use cases. Maybe that is intended, but it's not what I had > expected to see. I think your complaint is mostly related to the scope/goal of the overlayfs.rst file. The way it is currently written, and how the patch adds to it, it describes purely how overlayfs works "natively". For example, when you change just the permissions of a file from the lower layer, then overlayfs generates a metacopy file with some special xattrs in the upper dir (rather than a full copy). It can also do things like set a redirect xattr on the metacopy file if the file changes name in the upper (due to a rename for example). With the patch it may (depending on options) also set a verity xattr on the metacopy file so that later uses of the upper layer can ensure that the lower layer content file hasn't changed. This allows you to improve the robustness of overlayfs layers, such that if at a later time the lower directories accidentally change we will detect this when using the metacopy file rather than delivering the wrong data. These kinds of uses are completely standalone, you just mount overlay with the right options and the kernel will do the right thing. There is no need to know the internal details of the xattrs. The problem is that I also have a different usecase with composefs. This involves a completely different way of using overlayfs where you construct manually a filesystem with the xattrs that overlayfs reads, and then you mount this in a very special way (using a read-only filesystem on loopback). A combination of overlayfs features will give certain properties that are useful. I feel like you expect that the overlayfs.rst document should describe the details of the composefs setup, but instead it (both the patch and the existing document) mainly describes the other kind of use. In other words, if you are interested in using overlayfs with verity to increase the robustness against layer changes, then the document is probably enough. However, If you're interested in using overlayfs to implement more complex features like composefs, then the document is very weak in all sorts of details, not limited to the fs-verity part. So, I don't think the current document is bad for what it does. But it should perhaps be amended with a more detailed description of the internals of the xattrs and their behaviour so that advanced users (i.e. developers) don't have to read the source code for such details. That seems like a general critique though, and not necessarily related to this particular patchset. > Side note: the use of the passive voice, e.g. "the xattr may be set" and "the xattr may then be used to verify", should be avoided since it makes it unclear who/what is doing these actions. I'm not a native english speaker so I'm sure this can be written in a better way, but when it says: When metadata copy up is used for a file, then the xattr "trusted.overlay.verity" may be set on the metacopy file. It really means: In certain circumstances, depending on mount options and other details, it may be the case that when a metacopy file is being created by overlayfs (as a side effect of a filesystem operation) the xattr "trusted.overlay.verity" will be set on the new metacopy file. So, this is not really using "may" in the passive voice, but I see that it can be read that way. I'll try to reword this in a way that is more precise.
On Tue, Jun 13, 2023 at 9:37 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote: > > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > > > > +fs-verity support > > > > +---------------------- > > > > + > > > > +When metadata copy up is used for a file, then the xattr > > > > +"trusted.overlay.verity" may be set on the metacopy file. This > > > > +specifies the expected fs-verity digest of the lowerdata file. This > > > > +may then be used to verify the content of the source file at the time > > > > +the file is opened. During metacopy copy up overlayfs can also set > > > > +this xattr. > > > > + > > > > +This is controlled by the "verity" mount option, which supports > > > > +these values: > > > > + > > > > +- "off": > > > > + The verity xattr is never used. This is the default if verity > > > > + option is not specified. > > > > +- "on": > > > > + Whenever a metacopy files specifies an expected digest, the > > > > + corresponding data file must match the specified digest. > > > > + When generating a metacopy file the verity xattr will be set > > > > + from the source file fs-verity digest (if it has one). > > > > +- "require": > > > > + Same as "on", but additionally all metacopy files must specify a > > > > + verity xattr. This means metadata copy up will only be used if > > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > > + used. > > > > > > It looks like my request for improved documentation was not taken, which is > > > unfortunate and makes this patchset difficult to review. > > > > > > > Which one? > > IIRC, you had two requests. > > One very broad to get the overlayfs.rst document up-to-date: > > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/ > > That isn't an accurate summary of what I said. I actually pointed out two > specific things that are confusing specifically in the context of this feature. > > > But I assume you mean the specific request about this sentence: > > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/ > > And that was a third specific thing. I got a detailed response back > (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@mail.gmail.com), > which was helpful. Unfortunately, the information in that response hasn't yet > found its way into the documentation that is being proposed. This response is mostly about composefs and as Alex wrote these details have no place in overlayfs.rst and not related to the proposed feature, which stands alone even without composefs. > > In general the proposed documentation reads like the audience is overlayfs > developers. I never considered that overlayfs.rst is for an audience other than overlayfs developers or people that want to become overlayfs developers. It is not a user guide. If it were, it would have been a very bad one. > It doesn't describe the motivation for the feature or how to use it > in each of the two use cases. Maybe that is intended, but it's not what I had > expected to see. > Yeh, that's a valid point. That is what I wanted to know - what exactly is missing. I guess this is the documented motivation: "This may then be used to verify the content of the source file at the time the file is opened" but it does not tell a complete chain of trust story. How about something along the lines of: "In the case that the upper layer can be trusted not to be tampered with while overlayfs is offline and some of the lower layers cannot be trusted not to be tampered with, the "verity" feature can protect against offline modification to lower files, whose metadata has been copied up to the upper layer (a.k.a "metacopy" files) ...." It's generic language that what the patches do, regardless of the trust model of composefs and how it composes an overlayfs layers. > Side note: the use of the passive voice, e.g. "the xattr may be set" and "the > xattr may then be used to verify", should be avoided since it makes it unclear > who/what is doing these actions. > I am not a native English speaker and bad at documentation in general, so the only thing I can say is the passive-aggressive "patches welcome" ;-) Thanks, Amir.
On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote: > > > > In general the proposed documentation reads like the audience is overlayfs > > developers. > > I never considered that overlayfs.rst is for an audience other than > overlayfs developers or people that want to become overlayfs > developers. It is not a user guide. If it were, it would have been a > very bad one. > > > It doesn't describe the motivation for the feature or how to use it > > in each of the two use cases. Maybe that is intended, but it's not what I had > > expected to see. > > > > Yeh, that's a valid point. > That is what I wanted to know - what exactly is missing. > I guess this is the documented motivation: Sure, but even if the document is just for kernel developers, it should still describe motivation and use cases, as those are important for userstanding. > "This may then be used to verify the content of the source file at the time > the file is opened" > > but it does not tell a complete chain of trust story. > > How about something along the lines of: > > "In the case that the upper layer can be trusted not to be tampered > with while overlayfs is offline So *online* tampering of the upper layer is fine? > and some of the lower layers cannot > be trusted not to be tampered with, the "verity" feature can protect > against offline modification to lower files Data of lower files, not simply "lower files", right? Are *online* modifications to lower files indeed not in scope? If the feature "can protect", then under what circumstances does it protect, and under what circumstances what does it not protect? It would also be helpful to explain what specifically is meant by "protect". Does it mean that overlayfs prevents modifications to lower file data, or does it mean that overlayfs detects modifications to lower file data after they happen? If the latter, what happens when overlayfs detects a modification? What do userspace programs experience? > , whose metadata has been > copied up to the upper layer (a.k.a "metacopy" files) ...." > > It's generic language that what the patches do, regardless of the > trust model of composefs and how it composes an overlayfs layers. It's better, but it could use some more detail. See my comments above. - Eric
On Tue, Jun 13, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote: > > > > > > In general the proposed documentation reads like the audience is overlayfs > > > developers. > > > > I never considered that overlayfs.rst is for an audience other than > > overlayfs developers or people that want to become overlayfs > > developers. It is not a user guide. If it were, it would have been a > > very bad one. > > > > > It doesn't describe the motivation for the feature or how to use it > > > in each of the two use cases. Maybe that is intended, but it's not what I had > > > expected to see. > > > > > > > Yeh, that's a valid point. > > That is what I wanted to know - what exactly is missing. > > I guess this is the documented motivation: > > Sure, but even if the document is just for kernel developers, it should still > describe motivation and use cases, as those are important for userstanding. > > > "This may then be used to verify the content of the source file at the time > > the file is opened" > > > > but it does not tell a complete chain of trust story. > > > > How about something along the lines of: > > > > "In the case that the upper layer can be trusted not to be tampered > > with while overlayfs is offline > > So *online* tampering of the upper layer is fine? No, for the sake of this section, it would be easier to say that upper is completely trusted and completely under our control and that the feature only hardens overlayfs when lower is not under our full control. In the case of composefs it's actually two lower layers, one trusted and one not trusted (and an optional trusted upper), but it does not matter IMO for the sake of explaining the basic feature. > > > and some of the lower layers cannot > > be trusted not to be tampered with, the "verity" feature can protect > > against offline modification to lower files > > Data of lower files, not simply "lower files", right? Right. > > Are *online* modifications to lower files indeed not in scope? They are. doc should not mention online. > > If the feature "can protect", then under what circumstances does it protect, and > under what circumstances what does it not protect? > > It would also be helpful to explain what specifically is meant by "protect". > Does it mean that overlayfs prevents modifications to lower file data, or does > it mean that overlayfs detects modifications to lower file data after they > happen? If the latter, what happens when overlayfs detects a modification? > What do userspace programs experience? > > > , whose metadata has been > > copied up to the upper layer (a.k.a "metacopy" files) ...." > > > > It's generic language that what the patches do, regardless of the > > trust model of composefs and how it composes an overlayfs layers. > > It's better, but it could use some more detail. See my comments above. > Fair enough. Alex, Please incorporate Eric's feedback above to come up with a more detailed description than: +This can be useful as a way to validate that a lower directory matches +the correct upper when it is re-mounted later. In case you can +guarantee that a overlayfs directory with verity xattrs is not +tampered with (for example using dm-verity or fs-verity) this can even +be used to guarantee the validity of an untrusted lower directory. + On the specific points that Eric mentioned. Thanks, Amir.
On Wed, Jun 14, 2023 at 7:24 AM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Jun 13, 2023 at 9:22 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Tue, Jun 13, 2023 at 12:34:10PM +0300, Amir Goldstein wrote: > > > > > > > > In general the proposed documentation reads like the audience is overlayfs > > > > developers. > > > > > > I never considered that overlayfs.rst is for an audience other than > > > overlayfs developers or people that want to become overlayfs > > > developers. It is not a user guide. If it were, it would have been a > > > very bad one. > > > > > > > It doesn't describe the motivation for the feature or how to use it > > > > in each of the two use cases. Maybe that is intended, but it's not what I had > > > > expected to see. > > > > > > > > > > Yeh, that's a valid point. > > > That is what I wanted to know - what exactly is missing. > > > I guess this is the documented motivation: > > > > Sure, but even if the document is just for kernel developers, it should still > > describe motivation and use cases, as those are important for userstanding. > > > > > "This may then be used to verify the content of the source file at the time > > > the file is opened" > > > > > > but it does not tell a complete chain of trust story. > > > > > > How about something along the lines of: > > > > > > "In the case that the upper layer can be trusted not to be tampered > > > with while overlayfs is offline > > > > So *online* tampering of the upper layer is fine? > > No, for the sake of this section, it would be easier to say > that upper is completely trusted and completely under our > control and that the feature only hardens overlayfs when > lower is not under our full control. > > In the case of composefs it's actually two lower layers, one trusted > and one not trusted (and an optional trusted upper), but it does not > matter IMO for the sake of explaining the basic feature. > > > > > > and some of the lower layers cannot > > > be trusted not to be tampered with, the "verity" feature can protect > > > against offline modification to lower files > > > > Data of lower files, not simply "lower files", right? > > Right. > > > > > Are *online* modifications to lower files indeed not in scope? > > They are. doc should not mention online. > > > > > If the feature "can protect", then under what circumstances does it protect, and > > under what circumstances what does it not protect? > > > > It would also be helpful to explain what specifically is meant by "protect". > > Does it mean that overlayfs prevents modifications to lower file data, or does > > it mean that overlayfs detects modifications to lower file data after they > > happen? If the latter, what happens when overlayfs detects a modification? > > What do userspace programs experience? > > > > > , whose metadata has been > > > copied up to the upper layer (a.k.a "metacopy" files) ...." > > > > > > It's generic language that what the patches do, regardless of the > > > trust model of composefs and how it composes an overlayfs layers. > > > > It's better, but it could use some more detail. See my comments above. > > > > Fair enough. > > Alex, > > Please incorporate Eric's feedback above to come up with a more > detailed description than: > > +This can be useful as a way to validate that a lower directory matches > +the correct upper when it is re-mounted later. In case you can > +guarantee that a overlayfs directory with verity xattrs is not > +tampered with (for example using dm-verity or fs-verity) this can even > +be used to guarantee the validity of an untrusted lower directory. > + > > On the specific points that Eric mentioned. I think maybe it's best we hash this out here first. What do you think about this version: fs-verity support ---------------------- During metadata copy up of a lower file, if the source file has fs-verity enabled and overlay verity support is enabled, then the "trusted.overlay.verity" xattr is set on the new metacopy file. This specifies the expected fs-verity digest of the lowerdata file, which is used to verify the content of the lower file at the time the metacopy file is opened. When a layer containing verity xattrs is used, it means that any such metacopy file in the upper layer is guaranteed to match the content that was in the lower at the time of the copy-up. If at any time (during a mount, after a remount, etc) such a file in the lower is replaced or modified in any way, then opening the corresponding file on overlayfs will result in EIO and a detailed error printed to the kernel logs. (Actually, due to caching the overlayfs mount might still see the previous file contents after a lower file is replaced under an active mount, but it will never see the wrong data.) Verity can be used as a general robustness check to detect accidental changes in the overlayfs directories in use. But, with additional care it can also give more powerful guarantees. For example, if the upper layer is fully trusted (by using dm-verity or something similar), then an untrusted lower layer can be used to supply validated file content for all metacopy files. If additionally the untrusted lower directories are specified as "Data-only", then they can only supply such file content, and the entire mount can be trusted to match the upper layer.
On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote: > When a layer containing verity xattrs is used, it means that any > such metacopy file in the upper layer is guaranteed to match the > content that was in the lower at the time of the copy-up. If at any time > (during a mount, after a remount, etc) such a file in the lower is > replaced or modified in any way, then opening the corresponding file on > overlayfs will result in EIO and a detailed error printed to the kernel logs. > (Actually, due to caching the overlayfs mount might still see the previous > file contents after a lower file is replaced under an active mount, but > it will never see the wrong data.) Well, the key point of fsverity is that data is not verified until it is actually read. At open time, the fsverity file digest is made available in constant time, and overlayfs will verify that. However, invalid data blocks are not reported until the data is actually read. The error that applications get is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files So overlayfs might report EIO at open time, or it might not report an error until the modified data is read. And in the latter case, presumably the error seen by the application matches the one for using fsverity natively? You can link to the fsverity documentation somewhere if it would be helpful, but I'd still like the semantics of how this works on overlayfs to be documented. - Eric
On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote: > > When a layer containing verity xattrs is used, it means that any > > such metacopy file in the upper layer is guaranteed to match the > > content that was in the lower at the time of the copy-up. If at any time > > (during a mount, after a remount, etc) such a file in the lower is > > replaced or modified in any way, then opening the corresponding file on > > overlayfs will result in EIO and a detailed error printed to the kernel logs. > > (Actually, due to caching the overlayfs mount might still see the previous > > file contents after a lower file is replaced under an active mount, but > > it will never see the wrong data.) > > Well, the key point of fsverity is that data is not verified until it is > actually read. At open time, the fsverity file digest is made available in > constant time, and overlayfs will verify that. However, invalid data blocks are > not reported until the data is actually read. The error that applications get > is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at > https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files > > So overlayfs might report EIO at open time, or it might not report an error > until the modified data is read. And in the latter case, presumably the error > seen by the application matches the one for using fsverity natively? Yes, I'm aware of that, but do we need to describe this in the overlayfs documentation? The text I wrote is describing the behaviour that overlayfs adds to the mix, and I sort of assumed the late validation from fs-verity itself would be known about if the file already has fs-verity enabled. > You can link to the fsverity documentation somewhere if it would be helpful, but > I'd still like the semantics of how this works on overlayfs to be documented. I guess just adding a link to that is not that bad. What about: ---- When a layer containing verity xattrs is used, it means that any such metacopy file in the upper layer is guaranteed to match the content that was in the lower at the time of the copy-up. If at any time (during a mount, after a remount, etc) such a file in the lower is replaced or modified in any way, then opening the corresponding file on overlayfs will result in EIO and a detailed error printed to the kernel logs. (Actually, due to caching the overlayfs mount might still see the previous file contents after a lower file is replaced under an active mount, but it will never see the wrong data.) In addition fs-verity will do late validation of the file content, as described in :ref:`Documentation/filesystems/fsverity.rst <accessing_verity_files>`. ---
On Fri, Jun 16, 2023 at 10:11:06AM +0200, Alexander Larsson wrote: > On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote: > > > When a layer containing verity xattrs is used, it means that any > > > such metacopy file in the upper layer is guaranteed to match the > > > content that was in the lower at the time of the copy-up. If at any time > > > (during a mount, after a remount, etc) such a file in the lower is > > > replaced or modified in any way, then opening the corresponding file on > > > overlayfs will result in EIO and a detailed error printed to the kernel logs. > > > (Actually, due to caching the overlayfs mount might still see the previous > > > file contents after a lower file is replaced under an active mount, but > > > it will never see the wrong data.) > > > > Well, the key point of fsverity is that data is not verified until it is > > actually read. At open time, the fsverity file digest is made available in > > constant time, and overlayfs will verify that. However, invalid data blocks are > > not reported until the data is actually read. The error that applications get > > is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at > > https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files > > > > So overlayfs might report EIO at open time, or it might not report an error > > until the modified data is read. And in the latter case, presumably the error > > seen by the application matches the one for using fsverity natively? > > Yes, I'm aware of that, but do we need to describe this in the > overlayfs documentation? > The text I wrote is describing the behaviour that overlayfs adds to > the mix, and I sort of > assumed the late validation from fs-verity itself would be known about > if the file already > has fs-verity enabled. > > > You can link to the fsverity documentation somewhere if it would be helpful, but > > I'd still like the semantics of how this works on overlayfs to be documented. > > I guess just adding a link to that is not that bad. What about: > > ---- > When a layer containing verity xattrs is used, it means that any such > metacopy file in the upper layer is guaranteed to match the content > that was in the lower at the time of the copy-up. If at any time > (during a mount, after a remount, etc) such a file in the lower is > replaced or modified in any way, then opening the corresponding file > on overlayfs will result in EIO and a detailed error printed to the > kernel logs. (Actually, due to caching the overlayfs mount might > still see the previous file contents after a lower file is replaced > under an active mount, but it will never see the wrong data.) In > addition fs-verity will do late validation of the file content, as > described in :ref:`Documentation/filesystems/fsverity.rst > <accessing_verity_files>`. That still has the incorrect statement "If at any time (during a mount, after a remount, etc) such a file in the lower is replaced or modified in any way, then opening the corresponding file on overlayfs will result in EIO and a detailed error printed to the kernel logs." See my last email where I explained why that statement is not correct. - Eric
On Sat, Jun 17, 2023 at 9:47 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Fri, Jun 16, 2023 at 10:11:06AM +0200, Alexander Larsson wrote: > > On Fri, Jun 16, 2023 at 1:52 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > On Wed, Jun 14, 2023 at 09:57:41AM +0200, Alexander Larsson wrote: > > > > When a layer containing verity xattrs is used, it means that any > > > > such metacopy file in the upper layer is guaranteed to match the > > > > content that was in the lower at the time of the copy-up. If at any time > > > > (during a mount, after a remount, etc) such a file in the lower is > > > > replaced or modified in any way, then opening the corresponding file on > > > > overlayfs will result in EIO and a detailed error printed to the kernel logs. > > > > (Actually, due to caching the overlayfs mount might still see the previous > > > > file contents after a lower file is replaced under an active mount, but > > > > it will never see the wrong data.) > > > > > > Well, the key point of fsverity is that data is not verified until it is > > > actually read. At open time, the fsverity file digest is made available in > > > constant time, and overlayfs will verify that. However, invalid data blocks are > > > not reported until the data is actually read. The error that applications get > > > is EIO for syscalls, and SIGBUS for memory-mapped reads, as mentioned at > > > https://www.kernel.org/doc/html/latest/filesystems/fsverity.html#accessing-verity-files > > > > > > So overlayfs might report EIO at open time, or it might not report an error > > > until the modified data is read. And in the latter case, presumably the error > > > seen by the application matches the one for using fsverity natively? > > > > Yes, I'm aware of that, but do we need to describe this in the > > overlayfs documentation? > > The text I wrote is describing the behaviour that overlayfs adds to > > the mix, and I sort of > > assumed the late validation from fs-verity itself would be known about > > if the file already > > has fs-verity enabled. > > > > > You can link to the fsverity documentation somewhere if it would be helpful, but > > > I'd still like the semantics of how this works on overlayfs to be documented. > > > > I guess just adding a link to that is not that bad. What about: > > > > ---- > > When a layer containing verity xattrs is used, it means that any such > > metacopy file in the upper layer is guaranteed to match the content > > that was in the lower at the time of the copy-up. If at any time > > (during a mount, after a remount, etc) such a file in the lower is > > replaced or modified in any way, then opening the corresponding file > > on overlayfs will result in EIO and a detailed error printed to the > > kernel logs. (Actually, due to caching the overlayfs mount might > > still see the previous file contents after a lower file is replaced > > under an active mount, but it will never see the wrong data.) In > > addition fs-verity will do late validation of the file content, as > > described in :ref:`Documentation/filesystems/fsverity.rst > > <accessing_verity_files>`. > > That still has the incorrect statement "If at any time (during a mount, after a > remount, etc) such a file in the lower is replaced or modified in any way, then > opening the corresponding file on overlayfs will result in EIO and a detailed > error printed to the kernel logs." See my last email where I explained why that > statement is not correct. If the modification of the file happens via the kernel vfs API, then the new backing file will have either have no, or the wrong fs-verity digest, which will be reported by overlayfs on open. It is true that a modification on the block level which the vfs is unaware of will be reported at read time, by fs-verity (i.e. independent of overlayfs). This is what I meant by the "in addition .." part, but maybe that is not clear. I'll try to rephrase it.
diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst index 4f36b8919f7c..9497988557b9 100644 --- a/Documentation/filesystems/overlayfs.rst +++ b/Documentation/filesystems/overlayfs.rst @@ -406,6 +406,33 @@ when a "metacopy" file in one of the lower layers above it, has a "redirect" to the absolute path of the "lower data" file in the "data-only" lower layer. +fs-verity support +---------------------- + +When metadata copy up is used for a file, then the xattr +"trusted.overlay.verity" may be set on the metacopy file. This +specifies the expected fs-verity digest of the lowerdata file. This +may then be used to verify the content of the source file at the time +the file is opened. During metacopy copy up overlayfs can also set +this xattr. + +This is controlled by the "verity" mount option, which supports +these values: + +- "off": + The verity xattr is never used. This is the default if verity + option is not specified. +- "on": + Whenever a metacopy files specifies an expected digest, the + corresponding data file must match the specified digest. + When generating a metacopy file the verity xattr will be set + from the source file fs-verity digest (if it has one). +- "require": + Same as "on", but additionally all metacopy files must specify a + verity xattr. This means metadata copy up will only be used if + the data file has fs-verity enabled, otherwise a full copy-up is + used. + Sharing and copying layers -------------------------- diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h index c6c7d09b494e..95464a1cb371 100644 --- a/fs/overlayfs/ovl_entry.h +++ b/fs/overlayfs/ovl_entry.h @@ -13,6 +13,9 @@ struct ovl_config { bool redirect_dir; bool redirect_follow; const char *redirect_mode; + bool verity; + bool require_verity; + const char *verity_mode; bool index; bool uuid; bool nfs_export; diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c index d9be5d318e1b..f3b51fe59f68 100644 --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -244,6 +244,7 @@ static void ovl_free_fs(struct ovl_fs *ofs) kfree(ofs->config.upperdir); kfree(ofs->config.workdir); kfree(ofs->config.redirect_mode); + kfree(ofs->config.verity_mode); if (ofs->creator_cred) put_cred(ofs->creator_cred); kfree(ofs); @@ -334,6 +335,11 @@ static const char *ovl_redirect_mode_def(void) return ovl_redirect_dir_def ? "on" : "off"; } +static const char *ovl_verity_mode_def(void) +{ + return "off"; +} + static const char * const ovl_xino_str[] = { "off", "auto", @@ -383,6 +389,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry) seq_puts(m, ",volatile"); if (ofs->config.userxattr) seq_puts(m, ",userxattr"); + if (strcmp(ofs->config.verity_mode, ovl_verity_mode_def()) != 0) + seq_printf(m, ",verity=%s", ofs->config.verity_mode); return 0; } @@ -438,6 +446,7 @@ enum { OPT_METACOPY_ON, OPT_METACOPY_OFF, OPT_VOLATILE, + OPT_VERITY, OPT_ERR, }; @@ -460,6 +469,7 @@ static const match_table_t ovl_tokens = { {OPT_METACOPY_ON, "metacopy=on"}, {OPT_METACOPY_OFF, "metacopy=off"}, {OPT_VOLATILE, "volatile"}, + {OPT_VERITY, "verity=%s"}, {OPT_ERR, NULL} }; @@ -509,6 +519,21 @@ static int ovl_parse_redirect_mode(struct ovl_config *config, const char *mode) return 0; } +static int ovl_parse_verity_mode(struct ovl_config *config, const char *mode) +{ + if (strcmp(mode, "on") == 0) { + config->verity = true; + } else if (strcmp(mode, "require") == 0) { + config->verity = true; + config->require_verity = true; + } else if (strcmp(mode, "off") != 0) { + pr_err("bad mount option \"verity=%s\"\n", mode); + return -EINVAL; + } + + return 0; +} + static int ovl_parse_opt(char *opt, struct ovl_config *config) { char *p; @@ -520,6 +545,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (!config->redirect_mode) return -ENOMEM; + config->verity_mode = kstrdup(ovl_verity_mode_def(), GFP_KERNEL); + if (!config->verity_mode) + return -ENOMEM; + while ((p = ovl_next_opt(&opt)) != NULL) { int token; substring_t args[MAX_OPT_ARGS]; @@ -620,6 +649,13 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->userxattr = true; break; + case OPT_VERITY: + kfree(config->verity_mode); + config->verity_mode = match_strdup(&args[0]); + if (!config->verity_mode) + return -ENOMEM; + break; + default: pr_err("unrecognized mount option \"%s\" or missing value\n", p); @@ -651,6 +687,22 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) if (err) return err; + err = ovl_parse_verity_mode(config, config->verity_mode); + if (err) + return err; + + /* Resolve verity -> metacopy dependency */ + if (config->verity && !config->metacopy) { + /* Don't allow explicit specified conflicting combinations */ + if (metacopy_opt) { + pr_err("conflicting options: metacopy=off,verity=%s\n", + config->verity_mode); + return -EINVAL; + } + /* Otherwise automatically enable metacopy. */ + config->metacopy = true; + } + /* * This is to make the logic below simpler. It doesn't make any other * difference, since config->redirect_dir is only used for upper. @@ -665,6 +717,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) config->redirect_mode); return -EINVAL; } + if (config->verity && redirect_opt) { + pr_err("conflicting options: verity=%s,redirect_dir=%s\n", + config->verity_mode, config->redirect_mode); + return -EINVAL; + } if (redirect_opt) { /* * There was an explicit redirect_dir=... that resulted @@ -700,7 +757,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) } } - /* Resolve nfs_export -> !metacopy dependency */ + /* Resolve nfs_export -> !metacopy && !verity dependency */ if (config->nfs_export && config->metacopy) { if (nfs_export_opt && metacopy_opt) { pr_err("conflicting options: nfs_export=on,metacopy=on\n"); @@ -713,6 +770,14 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) */ pr_info("disabling nfs_export due to metacopy=on\n"); config->nfs_export = false; + } else if (config->verity) { + /* + * There was an explicit verity=.. that resulted + * in this conflict. + */ + pr_info("disabling nfs_export due to verity=%s\n", + config->verity_mode); + config->nfs_export = false; } else { /* * There was an explicit nfs_export=on that resulted @@ -724,7 +789,7 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) } - /* Resolve userxattr -> !redirect && !metacopy dependency */ + /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */ if (config->userxattr) { if (config->redirect_follow && redirect_opt) { pr_err("conflicting options: userxattr,redirect_dir=%s\n", @@ -735,6 +800,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config) pr_err("conflicting options: userxattr,metacopy=on\n"); return -EINVAL; } + if (config->verity) { + pr_err("conflicting options: userxattr,verity=%s\n", + config->verity_mode); + return -EINVAL; + } /* * Silently disable default setting of redirect and metacopy. * This shall be the default in the future as well: these
This adds the scaffolding (docs, config, mount options) for supporting the new overlay xattr "overlay.verity", which contains a fs-verity digest. This is used for metacopy files, and the actual fs-verity digest of the lowerdata file needs to match it. The mount option "verity" specifies how this xattr is handled. If you enable verity ("verity=on") all existing xattrs are validated before use, and during metacopy we generate verity xattr in the upper metacopy file (if the source file has verity enabled). This means later accesses can guarantee that the same data is used. Additionally you can use "verity=require". In this mode all metacopy files must have a valid verity xattr. For this to work metadata copy-up must be able to create a verity xattr (so that later accesses are validated). Therefore, in this mode, if the lower data file doesn't have fs-verity enabled we fall back to a full copy rather than a metacopy. Actual implementation follows in a separate commit. Signed-off-by: Alexander Larsson <alexl@redhat.com> --- Documentation/filesystems/overlayfs.rst | 27 +++++++++ fs/overlayfs/ovl_entry.h | 3 + fs/overlayfs/super.c | 74 ++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 2 deletions(-)