The channels array in the cfg80211_scan_request has a __counted_by attribute attached to it, which points to the n_channels variable. This attribute is used in bounds checking, and if it is not set before the array is filled, then the bounds sanitizer will issue a warning or a kernel panic if CONFIG_UBSAN_TRAP is set.
This patch sets the size of allocated memory as the initial value for n_channels. It is updated with the actual number of added elements after the array is filled.
Fixes: aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate") Cc: stable@vger.kernel.org Signed-off-by: Aleksei Vetrov vvvvvv@google.com --- Changes in v2: - Added Fixes tag and added stable to CC - Link to v1: https://lore.kernel.org/r/20241028-nl80211_parse_sched_scan-bounds-checker-f... --- net/wireless/nl80211.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index d7d099f7118ab5d5c745905abdea85d246c2b7b2..9b1b9dc5a7eb2a864da7b0212bc6a156b7757a9d 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -9776,6 +9776,7 @@ nl80211_parse_sched_scan(struct wiphy *wiphy, struct wireless_dev *wdev, request = kzalloc(size, GFP_KERNEL); if (!request) return ERR_PTR(-ENOMEM); + request->n_channels = n_channels;
if (n_ssids) request->ssids = (void *)request +
--- base-commit: 81983758430957d9a5cb3333fe324fd70cf63e7e change-id: 20241028-nl80211_parse_sched_scan-bounds-checker-fix-c5842f41b863
Best regards,
Hello everyone,
On Tue, Oct 29, 2024 at 01:22:11PM +0000, Aleksei Vetrov wrote:
The channels array in the cfg80211_scan_request has a __counted_by attribute attached to it, which points to the n_channels variable. This attribute is used in bounds checking, and if it is not set before the array is filled, then the bounds sanitizer will issue a warning or a kernel panic if CONFIG_UBSAN_TRAP is set.
This patch sets the size of allocated memory as the initial value for n_channels. It is updated with the actual number of added elements after the array is filled.
Fixes: aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate") Cc: stable@vger.kernel.org Signed-off-by: Aleksei Vetrov vvvvvv@google.com
Changes in v2:
- Added Fixes tag and added stable to CC
- Link to v1: https://lore.kernel.org/r/20241028-nl80211_parse_sched_scan-bounds-checker-f...
I would really appreciate it if someone take a look at this single line patch. It looks like v2 of this patch has slipped through the cracks...
Best regards, --- Aleksei Vetrov
On 11/4/2024 8:10 AM, Aleksei Vetrov wrote:
Hello everyone,
On Tue, Oct 29, 2024 at 01:22:11PM +0000, Aleksei Vetrov wrote:
The channels array in the cfg80211_scan_request has a __counted_by attribute attached to it, which points to the n_channels variable. This attribute is used in bounds checking, and if it is not set before the array is filled, then the bounds sanitizer will issue a warning or a kernel panic if CONFIG_UBSAN_TRAP is set.
This patch sets the size of allocated memory as the initial value for n_channels. It is updated with the actual number of added elements after the array is filled.
Fixes: aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate") Cc: stable@vger.kernel.org Signed-off-by: Aleksei Vetrov vvvvvv@google.com
Changes in v2:
- Added Fixes tag and added stable to CC
- Link to v1: https://lore.kernel.org/r/20241028-nl80211_parse_sched_scan-bounds-checker-f...
I would really appreciate it if someone take a look at this single line patch. It looks like v2 of this patch has slipped through the cracks...
It has not slipped through the cracks, it is being tracked in patchwork: https://patchwork.kernel.org/project/linux-wireless/patch/20241029-nl80211_p...
The wireless maintainers have a lot of work and it can take weeks to process new patches.
Have patience, /jeff
On Mon, Nov 04, 2024 at 09:10:15AM -0800, Jeff Johnson wrote:
It has not slipped through the cracks, it is being tracked in patchwork: https://patchwork.kernel.org/project/linux-wireless/patch/20241029-nl80211_p...
Today I learned of a new tool, thank you!
The wireless maintainers have a lot of work and it can take weeks to process new patches.
Have patience, /jeff
The initial response on v1 was almost instant, so I was surprised that v2 took much more time. However that response was from linux-hardening guys and I didn't took in account that now I need the maintainer response that will take much more time.
Thank you again for taking your time to look into my patch! -- Aleksei Vetrov
On 10/29/2024 6:22 AM, Aleksei Vetrov wrote:
The channels array in the cfg80211_scan_request has a __counted_by attribute attached to it, which points to the n_channels variable. This attribute is used in bounds checking, and if it is not set before the array is filled, then the bounds sanitizer will issue a warning or a kernel panic if CONFIG_UBSAN_TRAP is set.
This patch sets the size of allocated memory as the initial value for n_channels. It is updated with the actual number of added elements after the array is filled.
Fixes: aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate") Cc: stable@vger.kernel.org Signed-off-by: Aleksei Vetrov vvvvvv@google.com
Reviewed-by: Jeff Johnson quic_jjohnson@quicinc.com
And it is exactly this kind of issue why I'm not accepting any __counted_by() changes in ath.git without actually testing the code that is modified.
/jeff
On Mon, Nov 04, 2024 at 09:12:09AM -0800, Jeff Johnson wrote:
Reviewed-by: Jeff Johnson quic_jjohnson@quicinc.com
And it is exactly this kind of issue why I'm not accepting any __counted_by() changes in ath.git without actually testing the code that is modified.
However, I was really lucky that my setup used nl80211_parse_sched_scan during normal operations and triggered bound sanitizer. After the patch was developed, I accidently wiped my device and couldn't reproduce the bug again normally, so I had to use iw tool to trigger nl80211_parse_sched_scan manually to test it properly.
I looked for some tests that cover this function and that I can run on the device, but couldn't find any. It would be nice if you know about such tests, so I can check if there are any other places where bound sanitizer may be triggered. I only know syzkaller tool that may be used to get more kernel coverage in general.
Best regards, -- Aleksei Vetrov
Jeff Johnson quic_jjohnson@quicinc.com writes:
On 10/29/2024 6:22 AM, Aleksei Vetrov wrote:
The channels array in the cfg80211_scan_request has a __counted_by attribute attached to it, which points to the n_channels variable. This attribute is used in bounds checking, and if it is not set before the array is filled, then the bounds sanitizer will issue a warning or a kernel panic if CONFIG_UBSAN_TRAP is set.
This patch sets the size of allocated memory as the initial value for n_channels. It is updated with the actual number of added elements after the array is filled.
Fixes: aa4ec06c455d ("wifi: cfg80211: use __counted_by where appropriate") Cc: stable@vger.kernel.org Signed-off-by: Aleksei Vetrov vvvvvv@google.com
Reviewed-by: Jeff Johnson quic_jjohnson@quicinc.com
And it is exactly this kind of issue why I'm not accepting any __counted_by() changes in ath.git without actually testing the code that is modified.
That's a good rule. If we ever manage to write that "wireless cleanup policy" document this is something we should add there.
linux-stable-mirror@lists.linaro.org
-
Aleksei Vetrov -
Jeff Johnson -
Kalle Valo