linux-kernel - Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 1 Jun 2023 10:25:42 -0300
From:   Marc Dionne <marc.dionne@...istor.com>
To:     David Howells <dhowells@...hat.com>
Cc:     linux-afs@...ts.infradead.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] afs: Fix setting of mtime when creating a file/dir/symlink

On Wed, May 31, 2023 at 7:50 PM David Howells <dhowells@...hat.com> wrote:
>
>
> kafs incorrectly passes a zero mtime (ie. 1st Jan 1970) to the server when
> creating a file, dir or symlink because commit 52af7105eceb caused the
> mtime recorded in the afs_operation struct to be passed to the server, but
> didn't modify the afs_mkdir(), afs_create() and afs_symlink() functions to
> set it first.
>
> Those functions were written with the assumption that the mtime would be
> obtained from the server - but that fell foul of malsynchronised clocks, so
> it was decided that the mtime should be set from the client instead.
>
> Fix this by filling in op->mtime before calling the create op.
>
> Fixes: 52af7105eceb ("afs: Set mtime from the client for yfs create operations")
> Signed-off-by: David Howells <dhowells@...hat.com>
> cc: Marc Dionne <marc.dionne@...istor.com>
> cc: linux-afs@...ts.infradead.org
> cc: linux-fsdevel@...r.kernel.org
> ---
>  fs/afs/dir.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 4dd97afa536c..5219182e52e1 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c
> @@ -1358,6 +1358,7 @@ static int afs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
>         op->dentry      = dentry;
>         op->create.mode = S_IFDIR | mode;
>         op->create.reason = afs_edit_dir_for_mkdir;
> +       op->mtime       = current_time(dir);
>         op->ops         = &afs_mkdir_operation;
>         return afs_do_sync_operation(op);
>  }
> @@ -1661,6 +1662,7 @@ static int afs_create(struct mnt_idmap *idmap, struct inode *dir,
>         op->dentry      = dentry;
>         op->create.mode = S_IFREG | mode;
>         op->create.reason = afs_edit_dir_for_create;
> +       op->mtime       = current_time(dir);
>         op->ops         = &afs_create_operation;
>         return afs_do_sync_operation(op);
>
> @@ -1796,6 +1798,7 @@ static int afs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>         op->ops                 = &afs_symlink_operation;
>         op->create.reason       = afs_edit_dir_for_symlink;
>         op->create.symlink      = content;
> +       op->mtime               = current_time(dir);
>         return afs_do_sync_operation(op);
>
>  error:

The fix looks good, but as we discussed privately, the issue that this
fixes predates commit 52af7105eceb.  That commit only touched the yfs
client code and made it rely on the op mtime rather than letting the
server set the time. This made it inherit the issue that was already
present for the non yfs client code.

Marc