linux-kernel - Re: [PATCH][next] fs: omfs: Use flexible-array member in struct omfs

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202307170758.14B5D83D@keescook>
Date:   Mon, 17 Jul 2023 08:20:22 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Bob Copeland <me@...copeland.com>,
        linux-karma-devel@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] fs: omfs: Use flexible-array member in struct
 omfs_extent

On Fri, Jul 14, 2023 at 01:56:37PM -0600, Gustavo A. R. Silva wrote:
> There are some binary differences before and after changes, but this are
> expected due to the change in the size of 'struct omfs_extent' and the
> necessary adjusments.

For binary changes, I think commit logs should have more details. In
this case, I can figure it out:

> diff --git a/fs/omfs/file.c b/fs/omfs/file.c
> index de8f57ee39ec..6b580b9da8e3 100644
> --- a/fs/omfs/file.c
> +++ b/fs/omfs/file.c
> @@ -14,7 +14,7 @@ static u32 omfs_max_extents(struct omfs_sb_info *sbi, int offset)
>  {
>  	return (sbi->s_sys_blocksize - offset -
>  		sizeof(struct omfs_extent)) /
> -		sizeof(struct omfs_extent_entry) + 1;
> +		sizeof(struct omfs_extent_entry);
>  }

I think the original calculation meant to do:

  	return (sbi->s_sys_blocksize - offset -
  		(sizeof(struct omfs_extent) - sizeof(struct omfs_extent_entry))) /
		sizeof(struct omfs_extent_entry);

So this binary difference looks correct. I rebuilt before/after this
patch with omfs_max_extents() marked as noinline, and all the binary
changes were isolated here, and did exactly as expected: the first half
is 16 smaller (size of struct omfs_extent_entry), and the final +1 is
removed:

-     2e1:      lea    -0x20(%rbx),%rax
+     2e1:      lea    -0x10(%rbx),%rax
      2e5:      pop    %rbx
      2e6:      pop    %rbp
      2e7:      shr    $0x4,%rax
-     2eb:      add    $0x1,%eax

So this looks correct to me. Thanks!

Reviewed-by: Kees Cook <keescook@...omium.org>

-- 
Kees Cook