Re: [PATCH] ntfs: avoid leaking uninitialised bytes in new security descriptors

[Hyunchul Lee]

2026년 5월 8일 (금) 오전 12:48, DaeMyung Kang <charsyam@xxxxxxxxx>님이 작성:
>
> ntfs_sd_add_everyone() builds the on-disk security descriptor for a
> newly created file by kmalloc()'ing a buffer and then partially
> filling it in:
>
>         sd = kmalloc(sd_len, GFP_NOFS);
>         ...
>         sd->revision = 1;
>         sd->control = SE_DACL_PRESENT | SE_SELF_RELATIVE;
>         ...
>
> The buffer is then handed to ntfs_attr_add() and persisted as the
> SECURITY_DESCRIPTOR attribute of the new MFT record.  The descriptor
> covers a relative security descriptor header, two SIDs (owner and
> group), an ACL header, and a single ACE, but several fields inside
> those structures are never written before the buffer is committed
> to disk:
>
>   - struct security_descriptor_relative
>         @alignment              (1 byte)
>         @sacl                   (4 bytes; SE_SACL_PRESENT is not set
>                                  but the offset still reaches disk)
>
>   - struct ntfs_sid (3 instances: owner, group, ACE.sid)
>         identifier_authority.value[0..4] (5 bytes per SID, 15 total
>                                           - only value[5] is set)
>
>   - struct ntfs_acl
>         @alignment1             (1 byte)
>         @alignment2             (2 bytes)
>
> That is 23 bytes of uninitialised slab memory persisted to disk for
> every new file or directory the legacy ntfs driver creates.  The
> "+ 4" trailing accounting in sd_len holds ace->sid.sub_authority[0],
> which the existing code does explicitly write to zero, so it is
> not part of the leak.
>
> Anything later able to read the SECURITY_DESCRIPTOR attribute - the
> same NTFS volume mounted on Windows or by another NTFS reader, an
> offline forensics tool, an unprivileged user that ends up with read
> access to the volume - can recover those bytes.  The leak persists
> for the lifetime of the file on disk, not just the lifetime of the
> kernel that wrote it.
>
> Switch the allocation to kzalloc() so every byte the on-disk
> descriptor covers is zero before the explicit initialisations run.
> While there, replace the bare "return -1" allocation-failure path
> with a proper -ENOMEM so the error reaches userspace as a meaningful
> errno instead of an unrelated -EPERM.
>
> Found by inspection while auditing fs/ntfs new-inode paths.
>
> Fixes: af0db57d4293 ("ntfs: update inode operations")
> Signed-off-by: DaeMyung Kang <charsyam@xxxxxxxxx>

Looks good to me.

Reviewed-by: Hyunchul Lee <hyc.lee@xxxxxxxxx>

> ---
>  fs/ntfs/namei.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
> index dc786270afe5..aad928ebb7bd 100644
> --- a/fs/ntfs/namei.c
> +++ b/fs/ntfs/namei.c
> @@ -355,9 +355,9 @@ static int ntfs_sd_add_everyone(struct ntfs_inode *ni)
>         sd_len = sizeof(struct security_descriptor_relative) + 2 *
>                 (sizeof(struct ntfs_sid) + 8) + sizeof(struct ntfs_acl) +
>                 sizeof(struct ntfs_ace) + 4;
> -       sd = kmalloc(sd_len, GFP_NOFS);
> +       sd = kzalloc(sd_len, GFP_NOFS);
>         if (!sd)
> -               return -1;
> +               return -ENOMEM;
>
>         sd->revision = 1;
>         sd->control = SE_DACL_PRESENT | SE_SELF_RELATIVE;
> --
> 2.43.0
>


-- 
Thanks,
Hyunchul