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: <Z5PGvA-zoCV4rWii@aschofie-mobl2.lan>
Date: Fri, 24 Jan 2025 08:58:36 -0800
From: Alison Schofield <alison.schofield@...el.com>
To: Murad Masimov <m.masimov@...integration.ru>
Cc: Dave Jiang <dave.jiang@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	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" <nvdimm@...ts.linux.dev>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lvc-project@...uxtesting.org" <lvc-project@...uxtesting.org>,
	"stable@...r.kernel.org" <stable@...r.kernel.org>,
	"syzbot+c80d8dc0d9fa81a3cd8c@...kaller.appspotmail.com" <syzbot+c80d8dc0d9fa81a3cd8c@...kaller.appspotmail.com>
Subject: Re: [PATCH] acpi: nfit: fix narrowing conversion in acpi_nfit_ctl

On Fri, Jan 24, 2025 at 02:17:51PM +0000, Murad Masimov  wrote:
> 
> ________________________________________
> От: Dave Jiang <dave.jiang@...el.com>
> Отправлено: 24 января 2025 г. 2:43
> Кому: Masimov Murad; Dan Williams
> Копия: Vishal Verma; Ira Weiny; Rafael J. Wysocki; Len Brown; 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
> Тема: 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.
> 
> Thank you for the review! But I believe there is a misunderstanding. The point is that the code fragment affected by this patch is intended to make sure, that family is in range between 1 and NVDIMM_BUS_FAMILY_MAX. This is necessary because call_pkg contains user-provided data. However the implementation of these validity checks is erroneous and leads to passing an invalid value. The syzkaller report proves, that this bug can be triggered by a user. Here is an example to demonstrate, what exactly happens:
 > 
> 1. Let's say call_pkg->nd_family is equal to (1ull << 32).
> 2. Expression (cmd == ND_CMD_CALL && call_pkg->nd_family) evaluates to true.
> 3. Since family is of type int, and call_pkg->nd_family is u64, assigning call_pkg->nd_family to family will lead to a narrowing conversion.
> 4. As a result, family equals to 0, which will be passed in to_nfit_bus_uuid() triggering the warning.
> 
> Moreover, family may also be a negative integer (e.g. call_pkg->nd_family == ~(0ull)). This can lead to an undefined behaviour in test_bit() and potentially out-of-bounds in to_nfit_uuid(). Thus, even if triggering a WARN is not concerning, the bug still should be fixed.


Hi Murad,

Here's some general feedback for any Fix with a Fixes tag: the commit log
needs to state the user visible impact. State what user action fails and
include the actual error message that the user will see. That makes it
possible for folks to search and discover if a patch fixes their problem.

You've done some detailed sleuthing here but it's not clear that a
user would recognize this issue if they hit it.

--Alison


snip
>