linux-kernel - Re: [PATCH v2 4/4] riscv: Convert relocation iterator to do-while

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]

Date: Thu, 4 Jan 2024 15:35:55 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>,
	Palmer Dabbelt <palmer@...osinc.com>,
	linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel test robot <lkp@...el.com>,
	Dan Carpenter <error27@...il.com>
Subject: Re: [PATCH v2 4/4] riscv: Convert relocation iterator to do-while

On Wed, Jan 03, 2024 at 12:22:03PM -0800, Charlie Jenkins wrote:
> Use a do-while loop to iterate through relocation entries to prevent
> curr_type from being marked as uninitialized.
> 
> Signed-off-by: Charlie Jenkins <charlie@...osinc.com>
> Fixes: d8792a5734b0 ("riscv: Safely remove entries from relocation list")
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <error27@...il.com>
> Closes: https://lore.kernel.org/r/202312130859.wnkuzVWY-lkp@intel.com/
> ---
>  arch/riscv/kernel/module.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
> index ceb0adb38715..581e425686ab 100644
> --- a/arch/riscv/kernel/module.c
> +++ b/arch/riscv/kernel/module.c
> @@ -633,15 +633,31 @@ process_accumulated_relocations(struct module *me,
>  					  bucket_iter->bucket, node) {
>  			buffer = 0;
>  			location = rel_head_iter->location;
> -			list_for_each_entry_safe(rel_entry_iter,
> -						 rel_entry_iter_tmp,
> -						 rel_head_iter->rel_entry,
> -						 head) {
> +			rel_entry_iter =
> +				list_first_entry(rel_head_iter->rel_entry,
> +						 typeof(*rel_entry_iter), head);
> +			rel_entry_iter_tmp =
> +				list_next_entry(rel_entry_iter, head);
> +
> +			/*
> +			 * Iterate through all relocation entries that share
> +			 * this location. This uses a do-while loop instead of
> +			 * list_for_each_entry_safe since it is known that there
> +			 * is at least one entry and curr_type needs to be the
> +			 * value of the last entry when the loop exits.
> +			 */

I know that I reported this static checker and all, but actually after
reading this comment, I think we should stay with original code.  So
long as we know the list has "least one entry" which we do then the
original code worked fine.

To be honest, I probably would not have even reported this static
checker warning except that I saw there were some other issues and
thought "Eh, why not throw this warning in as well, in case the list
can be empty."

The other three patches look good.

regards,
dan carpenter