linux-kernel - Re: [PATCH] acpi: nfit: fix narrowing conversion in acpi_nfit

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: <649ed1bb-0686-42f0-802f-9f1909aeed8c@intel.com>
Date: Thu, 23 Jan 2025 16:43:21 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Murad Masimov <m.masimov@...integration.ru>,
 Dan Williams <dan.j.williams@...el.com>
Cc: Vishal Verma <vishal.l.verma@...el.com>, Ira Weiny <ira.weiny@...el.com>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 nvdimm@...ts.linux.dev, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org,
 stable@...r.kernel.org, syzbot+c80d8dc0d9fa81a3cd8c@...kaller.appspotmail.com
Subject: Re: [PATCH] acpi: nfit: fix narrowing conversion in acpi_nfit_ctl

On 1/23/25 9:39 AM, Murad Masimov wrote:
> Syzkaller has reported a warning in to_nfit_bus_uuid(): "only secondary
> bus families can be translated". This warning is emited if the argument
> is equal to NVDIMM_BUS_FAMILY_NFIT == 0. Function acpi_nfit_ctl() first
> verifies that a user-provided value call_pkg->nd_family of type u64 is
> not equal to 0. Then the value is converted to int, and only after that
> is compared to NVDIMM_BUS_FAMILY_MAX. This can lead to passing an invalid
> argument to acpi_nfit_ctl(), if call_pkg->nd_family is non-zero, while
> the lower 32 bits are zero.
> 
> All checks of the input value should be applied to the original variable
> call_pkg->nd_family.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: 6450ddbd5d8e ("ACPI: NFIT: Define runtime firmware activation commands")
> Cc: stable@...r.kernel.org
> Reported-by: syzbot+c80d8dc0d9fa81a3cd8c@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=c80d8dc0d9fa81a3cd8c
> Signed-off-by: Murad Masimov <m.masimov@...integration.ru>

While the change logically makes sense, the likelihood of nd_family > int_size is not ever likely. Given that NVDIMM_BUS_FAMILY_MAX is defined as 1, I don't think we care about values greater than that regardless of what is set in the upper 32bit of the u64. I'm leaning towards the fix is unnecessary.   

> ---
>  drivers/acpi/nfit/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index a5d47819b3a4..ae035b93da08 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -485,7 +485,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
>  		cmd_mask = nd_desc->cmd_mask;
>  		if (cmd == ND_CMD_CALL && call_pkg->nd_family) {
>  			family = call_pkg->nd_family;
> -			if (family > NVDIMM_BUS_FAMILY_MAX ||
> +			if (call_pkg->nd_family > NVDIMM_BUS_FAMILY_MAX ||
>  			    !test_bit(family, &nd_desc->bus_family_mask))
>  				return -EINVAL;
>  			family = array_index_nospec(family,
> --
> 2.39.2
>