| Message ID | 1679483469-2-1-git-send-email-ruansy.fnst@fujitsu.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | fsdax: unshare: zero destination if srcmap is HOLE or UNWRITTEN | expand |
On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > unshare copies data from source to destination. But if the source is > HOLE or UNWRITTEN extents, we should zero the destination, otherwise the > result will be unexpectable. Please provide much more detail on the user-visible effects of the bug. For example, are we leaking kernel memory contents to userspace? Thanks.
在 2023/3/23 7:03, Andrew Morton 写道: > On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > >> unshare copies data from source to destination. But if the source is >> HOLE or UNWRITTEN extents, we should zero the destination, otherwise the >> result will be unexpectable. > > Please provide much more detail on the user-visible effects of the bug. > For example, are we leaking kernel memory contents to userspace? This fixes fail of generic/649. -- Thanks, Ruan. > > Thanks. > >
On Thu, Mar 23, 2023 at 02:50:38PM +0800, Shiyang Ruan wrote: > > > 在 2023/3/23 7:03, Andrew Morton 写道: > > On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > > > unshare copies data from source to destination. But if the source is > > > HOLE or UNWRITTEN extents, we should zero the destination, otherwise the > > > result will be unexpectable. > > > > Please provide much more detail on the user-visible effects of the bug. > > For example, are we leaking kernel memory contents to userspace? > > This fixes fail of generic/649. Please make a note of easy(ish) reproducers when you know about them, e.g. "Found by running generic/649 while mounting with -o dax=always on pmem." So that anyone scanning around for fix backports has an easier time doing an A/B comparison to assess whether or not they need the fix. Especially if there's any chance that person is you circa 2024. ;) --D > > -- > Thanks, > Ruan. > > > > > Thanks. > > > >
On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > 在 2023/3/23 7:03, Andrew Morton 写道: > > On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > >> unshare copies data from source to destination. But if the source is > >> HOLE or UNWRITTEN extents, we should zero the destination, otherwise the > >> result will be unexpectable. > > > > Please provide much more detail on the user-visible effects of the bug. > > For example, are we leaking kernel memory contents to userspace? > > This fixes fail of generic/649. OK, but this doesn't really help. I'm trying to determine whether this fix should be backported into -stable kernels and whether it should be fast-tracked into Linus's current -rc tree. But to determine this I (and others) need to know what effect the bug has upon our users.
在 2023/3/24 6:11, Andrew Morton 写道: > On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > >> >> >> 在 2023/3/23 7:03, Andrew Morton 写道: >>> On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >>> >>>> unshare copies data from source to destination. But if the source is >>>> HOLE or UNWRITTEN extents, we should zero the destination, otherwise the >>>> result will be unexpectable. >>> >>> Please provide much more detail on the user-visible effects of the bug. >>> For example, are we leaking kernel memory contents to userspace? >> >> This fixes fail of generic/649. > > OK, but this doesn't really help. I'm trying to determine whether this > fix should be backported into -stable kernels and whether it should be > fast-tracked into Linus's current -rc tree. > > But to determine this I (and others) need to know what effect the bug > has upon our users. I didn't get any bug report form users. I just found this by running xfstests. The phenomenon of this problem is: if we funshare a reflinked file which contains HOLE extents, the result of the HOLE extents should be zero but actually not (unexpectable data). The other patch also has no reports from users. -- Thanks, Ruan.
On Fri, Mar 24, 2023 at 09:50:54AM +0800, Shiyang Ruan wrote: > > > 在 2023/3/24 6:11, Andrew Morton 写道: > > On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > > > > > > > > > 在 2023/3/23 7:03, Andrew Morton 写道: > > > > On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > > > > > > > unshare copies data from source to destination. But if the source is > > > > > HOLE or UNWRITTEN extents, we should zero the destination, otherwise the > > > > > result will be unexpectable. > > > > > > > > Please provide much more detail on the user-visible effects of the bug. > > > > For example, are we leaking kernel memory contents to userspace? > > > > > > This fixes fail of generic/649. > > > > OK, but this doesn't really help. I'm trying to determine whether this > > fix should be backported into -stable kernels and whether it should be > > fast-tracked into Linus's current -rc tree. > > > > But to determine this I (and others) need to know what effect the bug > > has upon our users. > > I didn't get any bug report form users. I just found this by running > xfstests. The phenomenon of this problem is: if we funshare a reflinked > file which contains HOLE extents, the result of the HOLE extents should be > zero but actually not (unexpectable data). You still aren't answering the question. If this did happen to a user, what would they see in the file? Random data? Something somebody else wrote some time ago? A copy of /etc/passwd, perhaps? A copy of your credit card number?
在 2023/3/24 11:33, Matthew Wilcox 写道: > On Fri, Mar 24, 2023 at 09:50:54AM +0800, Shiyang Ruan wrote: >> >> >> 在 2023/3/24 6:11, Andrew Morton 写道: >>> On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >>> >>>> >>>> >>>> 在 2023/3/23 7:03, Andrew Morton 写道: >>>>> On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >>>>> >>>>>> unshare copies data from source to destination. But if the source is >>>>>> HOLE or UNWRITTEN extents, we should zero the destination, otherwise the >>>>>> result will be unexpectable. >>>>> >>>>> Please provide much more detail on the user-visible effects of the bug. >>>>> For example, are we leaking kernel memory contents to userspace? >>>> >>>> This fixes fail of generic/649. >>> >>> OK, but this doesn't really help. I'm trying to determine whether this >>> fix should be backported into -stable kernels and whether it should be >>> fast-tracked into Linus's current -rc tree. >>> >>> But to determine this I (and others) need to know what effect the bug >>> has upon our users. >> >> I didn't get any bug report form users. I just found this by running >> xfstests. The phenomenon of this problem is: if we funshare a reflinked >> file which contains HOLE extents, the result of the HOLE extents should be >> zero but actually not (unexpectable data). > > You still aren't answering the question. If this did happen to a user, > what would they see in the file? Random data? Something somebody else > wrote some time ago? A copy of /etc/passwd, perhaps? A copy of your > credit card number? Ok. If this happenned to a user, the HOLE or UNWRITTEN part will be old data of the new allocated extent because it didn't be cleared. -- Thanks, Ruan.
On Fri, Mar 24, 2023 at 11:42:53AM +0800, Shiyang Ruan wrote: > > > 在 2023/3/24 11:33, Matthew Wilcox 写道: > > On Fri, Mar 24, 2023 at 09:50:54AM +0800, Shiyang Ruan wrote: > > > > > > > > > 在 2023/3/24 6:11, Andrew Morton 写道: > > > > On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > > > > > > > > > > > > > > > > > 在 2023/3/23 7:03, Andrew Morton 写道: > > > > > > On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > > > > > > > > > > > > unshare copies data from source to destination. But if the source is > > > > > > > HOLE or UNWRITTEN extents, we should zero the destination, otherwise the > > > > > > > result will be unexpectable. > > > > > > > > > > > > Please provide much more detail on the user-visible effects of the bug. > > > > > > For example, are we leaking kernel memory contents to userspace? > > > > > > > > > > This fixes fail of generic/649. > > > > > > > > OK, but this doesn't really help. I'm trying to determine whether this > > > > fix should be backported into -stable kernels and whether it should be > > > > fast-tracked into Linus's current -rc tree. > > > > > > > > But to determine this I (and others) need to know what effect the bug > > > > has upon our users. > > > > > > I didn't get any bug report form users. I just found this by running > > > xfstests. The phenomenon of this problem is: if we funshare a reflinked > > > file which contains HOLE extents, the result of the HOLE extents should be > > > zero but actually not (unexpectable data). > > > > You still aren't answering the question. If this did happen to a user, > > what would they see in the file? Random data? Something somebody else > > wrote some time ago? A copy of /etc/passwd, perhaps? A copy of your > > credit card number? > > Ok. If this happenned to a user, the HOLE or UNWRITTEN part will be old > data of the new allocated extent because it didn't be cleared. ie it's the data that was in whatever file happened to use that space last, so this is a security bug because it's a data leak, and a backport is needed, and you should have indicated that by putting a cc: stable tag on the patch?
在 2023/3/24 12:17, Matthew Wilcox 写道: > On Fri, Mar 24, 2023 at 11:42:53AM +0800, Shiyang Ruan wrote: >> >> >> 在 2023/3/24 11:33, Matthew Wilcox 写道: >>> On Fri, Mar 24, 2023 at 09:50:54AM +0800, Shiyang Ruan wrote: >>>> >>>> >>>> 在 2023/3/24 6:11, Andrew Morton 写道: >>>>> On Thu, 23 Mar 2023 14:50:38 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> 在 2023/3/23 7:03, Andrew Morton 写道: >>>>>>> On Wed, 22 Mar 2023 11:11:09 +0000 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >>>>>>> >>>>>>>> unshare copies data from source to destination. But if the source is >>>>>>>> HOLE or UNWRITTEN extents, we should zero the destination, otherwise the >>>>>>>> result will be unexpectable. >>>>>>> >>>>>>> Please provide much more detail on the user-visible effects of the bug. >>>>>>> For example, are we leaking kernel memory contents to userspace? >>>>>> >>>>>> This fixes fail of generic/649. >>>>> >>>>> OK, but this doesn't really help. I'm trying to determine whether this >>>>> fix should be backported into -stable kernels and whether it should be >>>>> fast-tracked into Linus's current -rc tree. >>>>> >>>>> But to determine this I (and others) need to know what effect the bug >>>>> has upon our users. >>>> >>>> I didn't get any bug report form users. I just found this by running >>>> xfstests. The phenomenon of this problem is: if we funshare a reflinked >>>> file which contains HOLE extents, the result of the HOLE extents should be >>>> zero but actually not (unexpectable data). >>> >>> You still aren't answering the question. If this did happen to a user, >>> what would they see in the file? Random data? Something somebody else >>> wrote some time ago? A copy of /etc/passwd, perhaps? A copy of your >>> credit card number? >> >> Ok. If this happenned to a user, the HOLE or UNWRITTEN part will be old >> data of the new allocated extent because it didn't be cleared. > > ie it's the data that was in whatever file happened to use that space > last, so this is a security bug because it's a data leak, and a backport > is needed, and you should have indicated that by putting a cc: stable > tag on the patch? Yes, cc stable is needed. Then should I send a new patch with the tag added? -- Thanks, Ruan.
On Fri, 24 Mar 2023 12:28:07 +0800 Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: > > ie it's the data that was in whatever file happened to use that space > > last, so this is a security bug because it's a data leak, and a backport > > is needed, and you should have indicated that by putting a cc: stable > > tag on the patch? > > Yes, cc stable is needed. Then should I send a new patch with the tag > added? Thanks. I updated the changelog and added cc:stable. : unshare copies data from source to destination. But if the source is : HOLE or UNWRITTEN extents, we should zero the destination, otherwise : the HOLE or UNWRITTEN part will be user-visible old data of the new : allocated extent because it wasn't cleared. : : Found by running generic/649 while mounting with -o dax=always on pmem.
diff --git a/fs/dax.c b/fs/dax.c index 9800b93ee14d..5d2e9b10030e 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -1258,15 +1258,20 @@ static s64 dax_unshare_iter(struct iomap_iter *iter) /* don't bother with blocks that are not shared to start with */ if (!(iomap->flags & IOMAP_F_SHARED)) return length; - /* don't bother with holes or unwritten extents */ - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) - return length; id = dax_read_lock(); ret = dax_iomap_direct_access(iomap, pos, length, &daddr, NULL); if (ret < 0) goto out_unlock; + /* zero the distance if srcmap is HOLE or UNWRITTEN */ + if (srcmap->flags & IOMAP_F_SHARED || srcmap->type == IOMAP_UNWRITTEN) { + memset(daddr, 0, length); + dax_flush(iomap->dax_dev, daddr, length); + ret = length; + goto out_unlock; + } + ret = dax_iomap_direct_access(srcmap, pos, length, &saddr, NULL); if (ret < 0) goto out_unlock;
unshare copies data from source to destination. But if the source is HOLE or UNWRITTEN extents, we should zero the destination, otherwise the result will be unexpectable. Fixes: d984648e428b ("fsdax,xfs: port unshare to fsdax") Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> --- fs/dax.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)