Skip to content

Commit 4edfcee

Browse files
moooyoYu Leng (from Dev Box)
andauthored
[Power Display] Built-in monitor blacklist to mitigate DDC/CI BSOD (#47556, #47968) (#48051)
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request Adds a built-in monitor blacklist that skips known-bad monitor models at the DDC/CI discovery stage — before PowerDisplay sends any capabilities request to the firmware. Matches by EdidId (the PnP manufacturer + product code from EDID). Two entries pre-populated from existing crash reports: | EdidId | Reported in | Notes | |-----------|-------------|------------------------------------------------------------------------| | `LTM2C02` | #47556 | Counterfeit-EDID LG 27MR400 (PnP claims Litemax 40" 2011, actual hw is LG 27" 2024) | | `GSM7714` | #47968 | LG UltraWide HDR WFHD | Also logs `[EdidId=…] [FriendlyName=…] [DevicePath=…]` immediately before each `GetCapabilitiesString*` syscall in `DdcCiController`. If the kernel call BSODs (`win32kfull` stack-cookie overrun), that is the last log line that survives — adding a new blacklist entry then takes one PR to `BuiltInMonitorBlacklist.json` instead of a memory-dump triage. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Mitigates: #47556 - [x] Mitigates: #47968 - [x] **Communication:** This is the "Add blacklist and prevent the API call on broken monitor" mitigation outlined in [#47556 (comment)](#47556 (comment)). The underlying `win32kfull!DdcciGetCapabilitiesStringFromMonitor` stack overrun is tracked separately by the Windows team. - [x] **Tests:** 6 new MSTest unit tests under `PowerDisplay.Lib.UnitTests`. All 120 PowerDisplay tests pass on x64 Debug. - [ ] **Localization:** N/A — this PR ships no end-user-facing strings (no UI surface). - [ ] **Dev docs:** N/A. - [ ] **New binaries:** N/A. Data ships as an `<EmbeddedResource>` inside the existing `PowerDisplay.Models.dll`; no new .dll, no installer change. - [ ] **Documentation updated:** N/A. <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments ### Threat model Some retail monitor firmwares ship non-conformant DDC/CI capabilities strings — typically oversized, not NUL-terminated, or otherwise malformed. When PowerDisplay calls `GetCapabilitiesStringLength` / `CapabilitiesRequestAndCapabilitiesReply` on these monitors, the request flows into `win32kfull!CPhysicalMonitorHandle::DdcciGetCapabilitiesStringFromMonitor`, which copies the reply into a stack buffer without bounding the length. The kernel detects its own stack cookie corruption on the epilogue check and `__fastfail`s with `BUGCHECK 0x139 / 2 (STACK_COOKIE_CHECK_FAILURE)`. The kernel-side overrun is a Windows defect, not PowerDisplay's, but PowerDisplay is currently the most widely deployed consumer of this API on hot-plug. Until the Windows team fixes the kernel, the only safe mitigation in user space is to **never call** the capabilities API on monitor models known to trigger it. ### Architecture | File | Role | |---|---| | `PowerDisplay.Models/MonitorBlacklistEntry.cs` | POCO: `{ edidId, comments }` | | `PowerDisplay.Models/BuiltInMonitorBlacklist.json` | Data file (embedded resource) | | `PowerDisplay.Models/BuiltInMonitorBlacklist.cs` | Lazy-loaded reader, AOT-safe (source-gen `JsonSerializerContext`); silent fallback to empty list on any IO/parse failure | | `PowerDisplay.Lib/Services/MonitorBlacklistService.cs` | `IsBlocked(monitorId)` — extracts EdidId via `MonitorIdentity.EdidIdFromMonitorId`, checks a `HashSet<string>` with `OrdinalIgnoreCase` | | `PowerDisplay/Helpers/MonitorManager.cs` | Filters QueryDisplayConfig inventory by EdidId **before** any controller (DDC/CI or WMI) is dispatched; logs each skip with `[MonitorBlacklist] Skipping ...` | | `PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs` | Logs EdidId + FriendlyName + DevicePath immediately before the first capabilities syscall per-monitor | Matching is **model-level granularity** (EdidId, e.g. `GSM7714`) rather than device-level (full DevicePath). One entry covers every physical port and every machine with the same monitor model. ### Adding new entries Once the kernel overrun is fixed this list can shrink. Until then, when a new BSOD surfaces: 1. User reports BSOD, attaches PowerDisplay log 2. The last line before the crash now reads `DDC: probing capabilities [EdidId=XXXXXX] [FriendlyName='...'] [DevicePath=...]` 3. Add `{ "edidId": "XXXXXX", "comments": "See #issue" }` to `BuiltInMonitorBlacklist.json` 4. Submit PR, ship in next release ### Scoped out (deliberate) A user-customized blacklist surface (Settings UI dialog) was prototyped and hit multiple WinUI 3 dialog parse / measure crashes inside a `ContentDialog`. The custom-list code path was reverted in favor of "built-in only" for v1 reliability. UI can be re-added later on top of this foundation if needed — the data model and service already support it. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed 1. **Unit tests:** `dotnet test` on `PowerDisplay.Lib.UnitTests` → **120/120 pass** (including 6 new tests for loader + service: built-in JSON loads, EdidIds are normalized to upper-case, no blank entries, `Lazy<>` cache returns same instance, `IsBlocked` returns `false` for empty built-in list and for unidentifiable monitor IDs). 2. **Clean x64 Debug build** via `tools\build\build-essentials.cmd` + per-project `tools\build\build.cmd`: - `PowerDisplay.Models` — 0 warning, 0 error - `PowerDisplay.Lib` — 0 warning, 0 error - `PowerDisplay` (app) — 0 warning, 0 error 3. **Smoke test (manual):** launched PowerDisplay, confirmed every connected monitor produces a `DDC: probing capabilities [EdidId=…] [FriendlyName=…] [DevicePath=…]` log line right before its caps request. 4. **Empty-list case:** `BuiltInMonitorBlacklist.json` with no entries → `IsBlocked` returns `false` for any input (verified via unit test). 5. **Non-empty case (manual):** temporarily added a connected monitor's EdidId to the built-in JSON → that monitor disappears from `MonitorManager.Monitors` and `[MonitorBlacklist] Skipping ...` log line appears; removed the entry → monitor reappears, all per-monitor settings intact. --------- Co-authored-by: Yu Leng (from Dev Box) <yuleng@microsoft.com>
1 parent 33cf699 commit 4edfcee

12 files changed

Lines changed: 361 additions & 0 deletions

File tree

.github/actions/spell-check/expect.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -954,6 +954,7 @@ keynum
954954
keyremaps
955955
keyring
956956
keyvault
957+
kfull
957958
KILLFOCUS
958959
killrunner
959960
kmph
@@ -1061,6 +1062,7 @@ lstrcmpi
10611062
lstrcpyn
10621063
lstrlen
10631064
LTEXT
1065+
LTM
10641066
LTRREADING
10651067
luid
10661068
LUMA
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Linq;
6+
using Microsoft.VisualStudio.TestTools.UnitTesting;
7+
using PowerDisplay.Models;
8+
9+
namespace PowerDisplay.UnitTests;
10+
11+
[TestClass]
12+
public class BuiltInMonitorBlacklistTests
13+
{
14+
[TestMethod]
15+
public void Entries_LoadsWithoutThrowing()
16+
{
17+
var entries = BuiltInMonitorBlacklist.Entries;
18+
19+
Assert.IsNotNull(entries);
20+
}
21+
22+
[TestMethod]
23+
public void Entries_AreNormalizedToUpperCase()
24+
{
25+
foreach (var entry in BuiltInMonitorBlacklist.Entries)
26+
{
27+
Assert.AreEqual(
28+
entry.EdidId,
29+
entry.EdidId.ToUpperInvariant(),
30+
$"Entry '{entry.EdidId}' is not normalized to uppercase.");
31+
Assert.AreEqual(
32+
entry.EdidId.Trim(),
33+
entry.EdidId,
34+
$"Entry '{entry.EdidId}' has untrimmed whitespace.");
35+
}
36+
}
37+
38+
[TestMethod]
39+
public void Entries_ContainNoEmptyEdidIds()
40+
{
41+
Assert.IsFalse(
42+
BuiltInMonitorBlacklist.Entries.Any(e => string.IsNullOrWhiteSpace(e.EdidId)),
43+
"Built-in list should never contain blank EdidId entries.");
44+
}
45+
46+
[TestMethod]
47+
public void Entries_AreCached()
48+
{
49+
var first = BuiltInMonitorBlacklist.Entries;
50+
var second = BuiltInMonitorBlacklist.Entries;
51+
52+
Assert.AreSame(first, second, "Entries should be returned from a cached Lazy<>.");
53+
}
54+
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using Microsoft.VisualStudio.TestTools.UnitTesting;
6+
using PowerDisplay.Common.Services;
7+
8+
namespace PowerDisplay.UnitTests;
9+
10+
[TestClass]
11+
public class MonitorBlacklistServiceTests
12+
{
13+
private const string SamplePathDel = @"\\?\DISPLAY#DELD1A8#5&abc123&0&UID12345#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}";
14+
private const string SamplePathBoe = @"\\?\DISPLAY#BOE0900#4&xyz&0&UID0";
15+
16+
[TestMethod]
17+
public void IsBlocked_EmptyBuiltIn_ReturnsFalse()
18+
{
19+
// Built-in list ships empty in this release, so the service should never block.
20+
var service = new MonitorBlacklistService();
21+
22+
Assert.IsFalse(service.IsBlocked(SamplePathDel));
23+
Assert.IsFalse(service.IsBlocked(SamplePathBoe));
24+
}
25+
26+
[TestMethod]
27+
public void IsBlocked_EmptyOrUnknownMonitorId_ReturnsFalse()
28+
{
29+
var service = new MonitorBlacklistService();
30+
31+
Assert.IsFalse(service.IsBlocked(string.Empty));
32+
Assert.IsFalse(service.IsBlocked(null!));
33+
Assert.IsFalse(service.IsBlocked(@"\\?\DISPLAY"));
34+
Assert.IsFalse(service.IsBlocked(@"garbage no hashes here"));
35+
}
36+
}

src/modules/powerdisplay/PowerDisplay.Lib/Drivers/DDC/DdcCiController.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,16 @@ private async Task<IReadOnlyList<Monitor>> DiscoverFromHandleAsync(
703703
}
704704
#endif
705705

706+
// Log identity of the monitor we are about to touch via DDC/CI BEFORE the
707+
// first syscall. If the call triggers a kernel stack-cookie overrun inside
708+
// win32kfull (see GH #47556 / #47968), this is the last log line that
709+
// survives — it has to carry enough to identify the offending hardware:
710+
// EdidId for blacklist matching, plus the human-readable name and full
711+
// DevicePath.
712+
var edidId = MonitorIdentity.EdidIdFromMonitorId(info.DevicePath);
713+
Logger.LogInfo(
714+
$"DDC: probing capabilities [EdidId={edidId}] [FriendlyName='{info.FriendlyName}'] [DevicePath={info.DevicePath}]");
715+
706716
// Async caps fetch (retry + max-compat probe). Awaits Task.Delay between
707717
// retries instead of blocking the threadpool.
708718
var (capsString, caps) = await FetchCapabilitiesWithFallbackAsync(
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.Collections.Generic;
7+
using PowerDisplay.Common.Models;
8+
using PowerDisplay.Models;
9+
10+
namespace PowerDisplay.Common.Services
11+
{
12+
/// <summary>
13+
/// Decides whether a monitor identified by its <c>Monitor.Id</c> (or raw DevicePath)
14+
/// should be filtered out of PowerDisplay's discovery. Matches on EdidId only —
15+
/// model-level granularity, so a single entry covers every physical port and every
16+
/// machine with the same monitor model.
17+
/// </summary>
18+
/// <remarks>
19+
/// Only the built-in list shipped with PowerToys is consulted; user-customized
20+
/// blacklists were considered but cut due to UI cost. EdidIds are normalized
21+
/// (trimmed, upper-cased) on construction; comparisons use
22+
/// <see cref="StringComparer.OrdinalIgnoreCase"/> as defense in depth.
23+
/// </remarks>
24+
public sealed class MonitorBlacklistService
25+
{
26+
private readonly HashSet<string> _blockedEdidIds;
27+
28+
public MonitorBlacklistService()
29+
{
30+
_blockedEdidIds = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
31+
32+
foreach (var entry in BuiltInMonitorBlacklist.Entries)
33+
{
34+
AddNormalized(entry.EdidId);
35+
}
36+
}
37+
38+
/// <summary>
39+
/// Returns true if <paramref name="monitorId"/> (a <c>Monitor.Id</c> or raw Windows
40+
/// DevicePath) has an EdidId in the built-in blacklist. Monitors whose EdidId cannot
41+
/// be extracted (empty path, malformed) are never blocked — we only filter what we
42+
/// can positively identify.
43+
/// </summary>
44+
public bool IsBlocked(string monitorId)
45+
{
46+
var edid = MonitorIdentity.EdidIdFromMonitorId(monitorId);
47+
return !string.IsNullOrEmpty(edid) && _blockedEdidIds.Contains(edid);
48+
}
49+
50+
private void AddNormalized(string? edidId)
51+
{
52+
var trimmed = edidId?.Trim();
53+
if (!string.IsNullOrEmpty(trimmed))
54+
{
55+
_blockedEdidIds.Add(trimmed.ToUpperInvariant());
56+
}
57+
}
58+
}
59+
}
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.Collections.Generic;
7+
using System.IO;
8+
using System.Linq;
9+
using System.Reflection;
10+
using System.Text.Json;
11+
12+
namespace PowerDisplay.Models
13+
{
14+
/// <summary>
15+
/// Loads the built-in monitor blacklist shipped with PowerToys.
16+
/// The data is an embedded JSON resource in this assembly; the file is read once
17+
/// on first access and cached for the lifetime of the process.
18+
/// </summary>
19+
/// <remarks>
20+
/// Loader failures are non-fatal: on any exception (missing resource, malformed
21+
/// JSON, etc.) the loader returns an empty list. This keeps PowerDisplay running
22+
/// even if a malformed release ships, and avoids logging dependencies inside the
23+
/// AOT-compatible PowerDisplay.Models assembly.
24+
/// </remarks>
25+
public static class BuiltInMonitorBlacklist
26+
{
27+
private const string ResourceName = "PowerDisplay.Models.BuiltInMonitorBlacklist.json";
28+
29+
private static readonly Lazy<IReadOnlyList<MonitorBlacklistEntry>> _entries
30+
= new(LoadFromResource);
31+
32+
public static IReadOnlyList<MonitorBlacklistEntry> Entries => _entries.Value;
33+
34+
private static IReadOnlyList<MonitorBlacklistEntry> LoadFromResource()
35+
{
36+
try
37+
{
38+
var assembly = typeof(BuiltInMonitorBlacklist).Assembly;
39+
using var stream = assembly.GetManifestResourceStream(ResourceName);
40+
if (stream == null)
41+
{
42+
return Array.Empty<MonitorBlacklistEntry>();
43+
}
44+
45+
var file = JsonSerializer.Deserialize(
46+
stream,
47+
MonitorBlacklistSerializationContext.Default.BuiltInMonitorBlacklistFile);
48+
49+
if (file?.Entries == null)
50+
{
51+
return Array.Empty<MonitorBlacklistEntry>();
52+
}
53+
54+
// Only the v1 schema is understood by this build. Future versions
55+
// ship a refreshed binary that updates this check.
56+
if (file.Version != 1)
57+
{
58+
return Array.Empty<MonitorBlacklistEntry>();
59+
}
60+
61+
return file.Entries
62+
.Where(e => !string.IsNullOrWhiteSpace(e.EdidId))
63+
.Select(e => new MonitorBlacklistEntry
64+
{
65+
EdidId = e.EdidId.Trim().ToUpperInvariant(),
66+
Comments = e.Comments ?? string.Empty,
67+
})
68+
.ToList();
69+
}
70+
catch
71+
{
72+
return Array.Empty<MonitorBlacklistEntry>();
73+
}
74+
}
75+
}
76+
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
{
2+
"version": 1,
3+
"entries": [
4+
{
5+
"edidId": "LTM2C02",
6+
"comments": "See https://github.com/microsoft/PowerToys/issues/47556"
7+
},
8+
{
9+
"edidId": "GSM7714",
10+
"comments": "See https://github.com/microsoft/PowerToys/issues/47968"
11+
}
12+
]
13+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Collections.Generic;
6+
using System.Text.Json.Serialization;
7+
8+
namespace PowerDisplay.Models
9+
{
10+
/// <summary>
11+
/// JSON file shape for <see cref="BuiltInMonitorBlacklist"/>.
12+
/// The <see cref="Version"/> field is a forward-compatibility marker; this
13+
/// release only understands version 1.
14+
/// </summary>
15+
public class BuiltInMonitorBlacklistFile
16+
{
17+
[JsonPropertyName("version")]
18+
public int Version { get; set; }
19+
20+
[JsonPropertyName("entries")]
21+
public List<MonitorBlacklistEntry> Entries { get; set; } = new();
22+
}
23+
}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Text.Json.Serialization;
6+
7+
namespace PowerDisplay.Models
8+
{
9+
/// <summary>
10+
/// One entry in a PowerDisplay monitor blacklist. Used both for the built-in
11+
/// list shipped with PowerToys (loaded via <see cref="BuiltInMonitorBlacklist"/>)
12+
/// and for the user-editable custom list persisted on <c>PowerDisplayProperties</c>.
13+
/// </summary>
14+
/// <remarks>
15+
/// <para><see cref="EdidId"/> is the 7–8 character PnP hardware identifier extracted
16+
/// from a <c>Monitor.Id</c> by <c>MonitorIdentity.EdidIdFromMonitorId</c> (e.g.
17+
/// <c>"DELD1A8"</c>, <c>"BOE0900"</c>). It is normalized to uppercase and trimmed
18+
/// on write; matching is case-insensitive as a defense-in-depth measure.</para>
19+
/// <para><see cref="Comments"/> is free text rendered as-is. The built-in JSON ships
20+
/// English-only comments; user input is not localized.</para>
21+
/// </remarks>
22+
public class MonitorBlacklistEntry
23+
{
24+
[JsonPropertyName("edidId")]
25+
public string EdidId { get; set; } = string.Empty;
26+
27+
[JsonPropertyName("comments")]
28+
public string Comments { get; set; } = string.Empty;
29+
}
30+
}
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright (c) Microsoft Corporation
2+
// The Microsoft Corporation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Collections.Generic;
6+
using System.Text.Json.Serialization;
7+
8+
namespace PowerDisplay.Models
9+
{
10+
/// <summary>
11+
/// JSON serialization context for monitor blacklist types.
12+
/// Provides source-generated serialization for Native AOT compatibility.
13+
/// </summary>
14+
[JsonSourceGenerationOptions(
15+
WriteIndented = false,
16+
DefaultIgnoreCondition = JsonIgnoreCondition.Never,
17+
IncludeFields = true)]
18+
[JsonSerializable(typeof(MonitorBlacklistEntry))]
19+
[JsonSerializable(typeof(List<MonitorBlacklistEntry>))]
20+
[JsonSerializable(typeof(BuiltInMonitorBlacklistFile))]
21+
public partial class MonitorBlacklistSerializationContext : JsonSerializerContext
22+
{
23+
}
24+
}

0 commit comments

Comments
 (0)