Fix handling of short discriminator in Linux BLE scan. (#21187)

* Fix handling of short discriminator in Linux BLE scan.

A few changes here:

1. Implement a SetupDiscriminator class that commons up logic like "does this
   discriminator, which might be short or long, match the given long
   discriminator?" and "extract short discriminator from long discriminator".
2. Change SetupPayload to more clearly indicate whether it's storing a short or
   long discriminator, instead of storing a short discriminator the same way as
   a long discriminator with the low bits all 0 and hoping consumers check
   isShortDiscriminator.
3. Update BLE scanning APIs to take SetupDiscriminator.
4. Fix the Linux and Tizen BLE code to properly handle short discriminators
   (which used to not match if the long discriminator happened to have the low 8
   bits nonzero).
5. Fix the Darwin BLE code to properly handle long discriminators that have 0
   low bits.  Before this change they used to incorrectly match a long
   discriminator which had the same 4 high bits but different 8 low bits.

Fixes #21160

* Address review comment.

examples/chip-tool/commands/payload/SetupPayloadGenerateCommand.cpp

@@ -30,7 +30,7 @@ void SetupPayloadGenerateCommand::ConfigurePayload(SetupPayload & payload)
 {
     if (mDiscriminator.HasValue())
     {
-        payload.discriminator = mDiscriminator.Value();
+        payload.discriminator.SetLongValue(mDiscriminator.Value());
     }
 
     if (mSetUpPINCode.HasValue())

examples/chip-tool/commands/payload/SetupPayloadParseCommand.cpp

@@ -70,10 +70,10 @@ CHIP_ERROR SetupPayloadParseCommand::Parse(std::string codeString, chip::SetupPa
 
 CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)
 {
-    ChipLogProgress(SetupPayload, "Version:       %u", payload.version);
-    ChipLogProgress(SetupPayload, "VendorID:      %u", payload.vendorID);
-    ChipLogProgress(SetupPayload, "ProductID:     %u", payload.productID);
-    ChipLogProgress(SetupPayload, "Custom flow:   %u    (%s)", to_underlying(payload.commissioningFlow),
+    ChipLogProgress(SetupPayload, "Version:             %u", payload.version);
+    ChipLogProgress(SetupPayload, "VendorID:            %u", payload.vendorID);
+    ChipLogProgress(SetupPayload, "ProductID:           %u", payload.productID);
+    ChipLogProgress(SetupPayload, "Custom flow:         %u    (%s)", to_underlying(payload.commissioningFlow),
                     CustomFlowString(payload.commissioningFlow));
     {
         StringBuilder<128> humanFlags;
@@ -106,15 +106,24 @@ CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)
             humanFlags.Add("NONE");
         }
 
-        ChipLogProgress(SetupPayload, "Capabilities:  0x%02X (%s)", payload.rendezvousInformation.Raw(), humanFlags.c_str());
+        ChipLogProgress(SetupPayload, "Capabilities:        0x%02X (%s)", payload.rendezvousInformation.Raw(), humanFlags.c_str());
     }
-    ChipLogProgress(SetupPayload, "Discriminator: %u", payload.discriminator);
-    ChipLogProgress(SetupPayload, "Passcode:      %u", payload.setUpPINCode);
+    if (payload.discriminator.IsShortDiscriminator())
+    {
+        ChipLogProgress(SetupPayload, "Short discriminator: %u   (0x%x)", payload.discriminator.GetShortValue(),
+                        payload.discriminator.GetShortValue());
+    }
+    else
+    {
+        ChipLogProgress(SetupPayload, "Long discriminator:  %u   (0x%x)", payload.discriminator.GetLongValue(),
+                        payload.discriminator.GetLongValue());
+    }
+    ChipLogProgress(SetupPayload, "Passcode:            %u", payload.setUpPINCode);
 
     std::string serialNumber;
     if (payload.getSerialNumber(serialNumber) == CHIP_NO_ERROR)
     {
-        ChipLogProgress(SetupPayload, "SerialNumber: %s", serialNumber.c_str());
+        ChipLogProgress(SetupPayload, "SerialNumber:        %s", serialNumber.c_str());
     }
 
     std::vector<OptionalQRCodeInfo> optionalVendorData = payload.getAllOptionalVendorData();
@@ -126,11 +135,11 @@ CHIP_ERROR SetupPayloadParseCommand::Print(chip::SetupPayload payload)
 
         if (isTypeString)
         {
-            ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,string value=%s", info.tag, info.data.c_str());
+            ChipLogProgress(SetupPayload, "OptionalQRCodeInfo:  tag=%u,string value=%s", info.tag, info.data.c_str());
         }
         else
         {
-            ChipLogProgress(SetupPayload, "OptionalQRCodeInfo: tag=%u,int value=%u", info.tag, info.int32);
+            ChipLogProgress(SetupPayload, "OptionalQRCodeInfo:  tag=%u,int value=%u", info.tag, info.int32);
         }
     }
 

examples/platform/linux/CommissionableInit.cpp

@@ -59,7 +59,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
 
     if (options.discriminator.HasValue())
     {
-        options.payload.discriminator = options.discriminator.Value();
+        options.payload.discriminator.SetLongValue(options.discriminator.Value());
     }
     else
     {
@@ -72,7 +72,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
                      "given on command line. This is temporary and will disappear. Please update your scripts "
                      "to explicitly configure discriminator. ***",
                      static_cast<unsigned>(defaultTestDiscriminator));
-        options.payload.discriminator = defaultTestDiscriminator;
+        options.payload.discriminator.SetLongValue(defaultTestDiscriminator);
     }
 
     // Default to minimum PBKDF iterations
@@ -84,7 +84,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
     ChipLogError(Support, "PASE PBKDF iterations set to %u", static_cast<unsigned>(spake2pIterationCount));
 
     return provider.Init(options.spake2pVerifier, options.spake2pSalt, spake2pIterationCount, setupPasscode,
-                         options.payload.discriminator);
+                         options.payload.discriminator.GetLongValue());
 }
 
 CHIP_ERROR InitConfigurationManager(ConfigurationManagerImpl & configManager, LinuxDeviceOptions & options)

examples/tv-casting-app/linux/main.cpp

@@ -85,7 +85,7 @@ CHIP_ERROR InitCommissionableDataProvider(LinuxCommissionableDataProvider & prov
     ChipLogError(Support, "PASE PBKDF iterations set to %u", static_cast<unsigned>(spake2pIterationCount));
 
     return provider.Init(options.spake2pVerifier, options.spake2pSalt, spake2pIterationCount, setupPasscode,
-                         options.payload.discriminator);
+                         options.payload.discriminator.GetLongValue());
 }
 
 // To hold SPAKE2+ verifier, discriminator, passcode

src/app/server/OnboardingCodesUtil.cpp

@@ -115,13 +115,15 @@ CHIP_ERROR GetPayloadContents(chip::PayloadContents & aPayload, chip::Rendezvous
 #endif
     }
 
-    err = GetCommissionableDataProvider()->GetSetupDiscriminator(aPayload.discriminator);
+    uint16_t discriminator = 0;
+    err                    = GetCommissionableDataProvider()->GetSetupDiscriminator(discriminator);
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(AppServer, "GetCommissionableDataProvider()->GetSetupDiscriminator() failed: %" CHIP_ERROR_FORMAT,
                      err.Format());
         return err;
     }
+    aPayload.discriminator.SetLongValue(discriminator);
 
     err = chip::DeviceLayer::GetDeviceInstanceInfoProvider()->GetVendorId(aPayload.vendorID);
     if (err != CHIP_NO_ERROR)

src/ble/BleConnectionDelegate.h

@@ -27,6 +27,7 @@
 #include <ble/BleError.h>
 
 #include <lib/support/DLLUtil.h>
+#include <lib/support/SetupDiscriminator.h>
 
 namespace chip {
 namespace Ble {
@@ -51,8 +52,8 @@ class DLL_EXPORT BleConnectionDelegate
     OnConnectionErrorFunct OnConnectionError;
 
     // Call this function to delegate the connection steps required to get a BLE_CONNECTION_OBJECT
-    // out of a peripheral discriminator.
-    virtual void NewConnection(BleLayer * bleLayer, void * appState, uint16_t connDiscriminator) = 0;
+    // out of a peripheral that matches the given discriminator.
+    virtual void NewConnection(BleLayer * bleLayer, void * appState, const SetupDiscriminator & connDiscriminator) = 0;
 
     // Call this function to stop the connection
     virtual CHIP_ERROR CancelConnection() = 0;

src/ble/BleLayer.cpp

@@ -371,7 +371,7 @@ CHIP_ERROR BleLayer::CancelBleIncompleteConnection()
     return err;
 }
 
-CHIP_ERROR BleLayer::NewBleConnectionByDiscriminator(uint16_t connDiscriminator, void * appState,
+CHIP_ERROR BleLayer::NewBleConnectionByDiscriminator(const SetupDiscriminator & connDiscriminator, void * appState,
                                                      BleConnectionDelegate::OnConnectionCompleteFunct onSuccess,
                                                      BleConnectionDelegate::OnConnectionErrorFunct onError)
 {

src/ble/BleLayer.h

@@ -54,6 +54,7 @@
 
 #include <ble/BleConfig.h>
 
+#include <lib/support/SetupDiscriminator.h>
 #include <system/SystemLayer.h>
 #include <system/SystemPacketBuffer.h>
 
@@ -245,7 +246,7 @@ class DLL_EXPORT BleLayer
     void Shutdown();
 
     CHIP_ERROR CancelBleIncompleteConnection();
-    CHIP_ERROR NewBleConnectionByDiscriminator(uint16_t connDiscriminator, void * appState = nullptr,
+    CHIP_ERROR NewBleConnectionByDiscriminator(const SetupDiscriminator & connDiscriminator, void * appState = nullptr,
                                                BleConnectionDelegate::OnConnectionCompleteFunct onSuccess = OnConnectionComplete,
                                                BleConnectionDelegate::OnConnectionErrorFunct onError      = OnConnectionError);
     CHIP_ERROR NewBleConnectionByObject(BLE_CONNECTION_OBJECT connObj);

src/controller/CHIPDeviceController.cpp

@@ -684,7 +684,9 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
         }
         else if (params.HasDiscriminator())
         {
-            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(params.GetDiscriminator()));
+            SetupDiscriminator discriminator;
+            discriminator.SetLongValue(params.GetDiscriminator());
+            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(discriminator));
         }
         else
         {

src/controller/CommissioningWindowOpener.cpp

@@ -96,8 +96,8 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
         mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer);
     }
 
-    mSetupPayload.version               = 0;
-    mSetupPayload.discriminator         = discriminator;
+    mSetupPayload.version = 0;
+    mSetupPayload.discriminator.SetLongValue(discriminator);
     mSetupPayload.rendezvousInformation = RendezvousInformationFlags(RendezvousInformationFlag::kOnNetwork);
 
     mCommissioningWindowCallback      = callback;
@@ -142,7 +142,7 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Operationa
         AdministratorCommissioning::Commands::OpenCommissioningWindow::Type request;
         request.commissioningTimeout = mCommissioningWindowTimeout.count();
         request.PAKEVerifier         = serializedVerifierSpan;
-        request.discriminator        = mSetupPayload.discriminator;
+        request.discriminator        = mSetupPayload.discriminator.GetLongValue();
         request.iterations           = mPBKDFIterations;
         request.salt                 = mPBKDFSalt;
 

src/controller/SetUpCodePairer.cpp

@@ -154,10 +154,17 @@ CHIP_ERROR SetUpCodePairer::StartDiscoverOverIP(SetupPayload & payload)
 {
     ChipLogProgress(Controller, "Starting commissioning discovery over DNS-SD");
 
-    currentFilter.type = payload.isShortDiscriminator ? Dnssd::DiscoveryFilterType::kShortDiscriminator
-                                                      : Dnssd::DiscoveryFilterType::kLongDiscriminator;
-    currentFilter.code =
-        payload.isShortDiscriminator ? static_cast<uint16_t>((payload.discriminator >> 8) & 0x0F) : payload.discriminator;
+    auto & discriminator = payload.discriminator;
+    if (discriminator.IsShortDiscriminator())
+    {
+        currentFilter.type = Dnssd::DiscoveryFilterType::kShortDiscriminator;
+        currentFilter.code = discriminator.GetShortValue();
+    }
+    else
+    {
+        currentFilter.type = Dnssd::DiscoveryFilterType::kLongDiscriminator;
+        currentFilter.code = discriminator.GetLongValue();
+    }
     // Handle possibly-sync callbacks.
     mWaitingForDiscovery[kIPTransport] = true;
     CHIP_ERROR err                     = mCommissioner->DiscoverCommissionableNodes(currentFilter);

src/controller/python/chip/setup_payload/Generator.cpp

@@ -35,11 +35,11 @@ extern "C" ChipError::StorageType pychip_SetupPayload_PrintOnboardingCodes(uint3
     SetupPayload payload;
     RendezvousInformationFlags rendezvousFlags = RendezvousInformationFlag::kNone;
 
-    payload.version               = version;
-    payload.setUpPINCode          = passcode;
-    payload.vendorID              = vendorId;
-    payload.productID             = productId;
-    payload.discriminator         = discriminator;
+    payload.version      = version;
+    payload.setUpPINCode = passcode;
+    payload.vendorID     = vendorId;
+    payload.productID    = productId;
+    payload.discriminator.SetLongValue(discriminator);
     payload.rendezvousInformation = rendezvousFlags.SetRaw(capabilities);
 
     switch (customFlow)

src/controller/python/chip/setup_payload/Parser.cpp

@@ -39,7 +39,14 @@ void YieldSetupPayloadAttributes(const SetupPayload & payload, AttributeVisitor
     attrVisitor("ProductID", std::to_string(payload.productID).c_str());
     attrVisitor("CommissioningFlow", std::to_string(static_cast<uint8_t>(payload.commissioningFlow)).c_str());
     attrVisitor("RendezvousInformation", std::to_string(payload.rendezvousInformation.Raw()).c_str());
-    attrVisitor("Discriminator", std::to_string(payload.discriminator).c_str());
+    if (payload.discriminator.IsShortDiscriminator())
+    {
+        attrVisitor("Short discriminator", std::to_string(payload.discriminator.GetShortValue()).c_str());
+    }
+    else
+    {
+        attrVisitor("Long discriminator", std::to_string(payload.discriminator.GetLongValue()).c_str());
+    }
     attrVisitor("SetUpPINCode", std::to_string(payload.setUpPINCode).c_str());
 
     std::string serialNumber;

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -350,7 +350,7 @@ - (BOOL)pairDevice:(uint64_t)deviceID
 
         std::string manualPairingCode;
         chip::SetupPayload payload;
-        payload.discriminator = discriminator;
+        payload.discriminator.SetLongValue(discriminator);
         payload.setUpPINCode = setupPINCode;
 
         auto errorCode = chip::ManualSetupPayloadGenerator(payload).payloadDecimalStringRepresentation(manualPairingCode);

src/darwin/Framework/CHIP/MTRSetupPayload.h

@@ -57,6 +57,7 @@ typedef NS_ENUM(NSUInteger, MTROptionalQRCodeInfoType) {
 @property (nonatomic, assign) MTRCommissioningFlow commissioningFlow;
 @property (nonatomic, assign) MTRRendezvousInformationFlags rendezvousInformation;
 @property (nonatomic, strong) NSNumber * discriminator;
+@property (nonatomic) BOOL hasShortDiscriminator;
 @property (nonatomic, strong) NSNumber * setUpPINCode;
 
 @property (nonatomic, strong) NSString * serialNumber;

src/darwin/Framework/CHIP/MTRSetupPayload.mm

@@ -64,7 +64,12 @@ - (id)initWithSetupPayload:(chip::SetupPayload)setupPayload
         _productID = [NSNumber numberWithUnsignedShort:setupPayload.productID];
         _commissioningFlow = [self convertCommissioningFlow:setupPayload.commissioningFlow];
         _rendezvousInformation = [self convertRendezvousFlags:setupPayload.rendezvousInformation];
-        _discriminator = [NSNumber numberWithUnsignedShort:setupPayload.discriminator];
+        _hasShortDiscriminator = setupPayload.discriminator.IsShortDiscriminator();
+        if (_hasShortDiscriminator) {
+            _discriminator = [NSNumber numberWithUnsignedShort:setupPayload.discriminator.GetShortValue()];
+        } else {
+            _discriminator = [NSNumber numberWithUnsignedShort:setupPayload.discriminator.GetLongValue()];
+        }
         _setUpPINCode = [NSNumber numberWithUnsignedInt:setupPayload.setUpPINCode];
 
         [self getSerialNumber:setupPayload];
@@ -134,6 +139,7 @@ + (NSUInteger)generateRandomPIN
 static NSString * const MTRSetupPayloadCodingKeyProductID = @"MTRSP.ck.productID";
 static NSString * const MTRSetupPayloadCodingKeyCommissioningFlow = @"MTRSP.ck.commissioningFlow";
 static NSString * const MTRSetupPayloadCodingKeyRendezvousFlags = @"MTRSP.ck.rendezvousFlags";
+static NSString * const MTRSetupPayloadCodingKeyHasShortDiscriminator = @"MTRSP.ck.hasShortDiscriminator";
 static NSString * const MTRSetupPayloadCodingKeyDiscriminator = @"MTRSP.ck.discriminator";
 static NSString * const MTRSetupPayloadCodingKeySetupPINCode = @"MTRSP.ck.setupPINCode";
 static NSString * const MTRSetupPayloadCodingKeySerialNumber = @"MTRSP.ck.serialNumber";
@@ -148,10 +154,11 @@ - (void)encodeWithCoder:(NSCoder *)coder
     [coder encodeObject:self.version forKey:MTRSetupPayloadCodingKeyVersion];
     [coder encodeObject:self.vendorID forKey:MTRSetupPayloadCodingKeyVendorID];
     [coder encodeObject:self.productID forKey:MTRSetupPayloadCodingKeyProductID];
-    // Casts are safe because commissioning flow and rendezvous information
-    // values are all pretty small and non-negative.
+    // Casts are safe because commissioning flow, rendezvous information, and
+    // hasShortDiscriminator values are all pretty small and non-negative.
     [coder encodeInteger:static_cast<NSInteger>(self.commissioningFlow) forKey:MTRSetupPayloadCodingKeyCommissioningFlow];
     [coder encodeInteger:static_cast<NSInteger>(self.rendezvousInformation) forKey:MTRSetupPayloadCodingKeyRendezvousFlags];
+    [coder encodeInteger:static_cast<NSInteger>(self.hasShortDiscriminator) forKey:MTRSetupPayloadCodingKeyHasShortDiscriminator];
     [coder encodeObject:self.discriminator forKey:MTRSetupPayloadCodingKeyDiscriminator];
     [coder encodeObject:self.setUpPINCode forKey:MTRSetupPayloadCodingKeySetupPINCode];
     [coder encodeObject:self.serialNumber forKey:MTRSetupPayloadCodingKeySerialNumber];
@@ -164,6 +171,7 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder
     NSNumber * productID = [decoder decodeObjectOfClass:[NSNumber class] forKey:MTRSetupPayloadCodingKeyProductID];
     NSInteger commissioningFlow = [decoder decodeIntegerForKey:MTRSetupPayloadCodingKeyCommissioningFlow];
     NSInteger rendezvousInformation = [decoder decodeIntegerForKey:MTRSetupPayloadCodingKeyRendezvousFlags];
+    NSInteger hasShortDiscriminator = [decoder decodeIntegerForKey:MTRSetupPayloadCodingKeyHasShortDiscriminator];
     NSNumber * discriminator = [decoder decodeObjectOfClass:[NSNumber class] forKey:MTRSetupPayloadCodingKeyDiscriminator];
     NSNumber * setUpPINCode = [decoder decodeObjectOfClass:[NSNumber class] forKey:MTRSetupPayloadCodingKeySetupPINCode];
     NSString * serialNumber = [decoder decodeObjectOfClass:[NSString class] forKey:MTRSetupPayloadCodingKeySerialNumber];
@@ -174,6 +182,7 @@ - (nullable instancetype)initWithCoder:(NSCoder *)decoder
     payload.productID = productID;
     payload.commissioningFlow = static_cast<MTRCommissioningFlow>(commissioningFlow);
     payload.rendezvousInformation = static_cast<MTRRendezvousInformationFlags>(rendezvousInformation);
+    payload.hasShortDiscriminator = static_cast<BOOL>(hasShortDiscriminator);
     payload.discriminator = discriminator;
     payload.setUpPINCode = setUpPINCode;
     payload.serialNumber = serialNumber;