Skip to content

Commit 89d6c99

Browse files
benmabenma
committed
api/firmware: make sure a series of request doesn't get interrupted
Some API calls, like BTCSign, or BluetoothUpgrade, require a series of request<>response pairs. If the device gets an unexpected message, it fails. However, this is not concurrency safe - if another goroutine sends something in the middle of it, the API call in progress and the interrupting call both fail. To solve this, we introduce `apiLock` that ensures a series of queries does not get interrupted. Most API functions only use a single query, so we acquire this lock by default in `.query()`. We add a `nonAtomicQuery()` variant with a `atomicQueries()` helper function to enable making a series of calls.
1 parent 8f5c10a commit 89d6c99

File tree

4 files changed

+136
-62
lines changed

4 files changed

+136
-62
lines changed

‎api/firmware/bluetooth.go

+35-37
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@ import (
1919
"github.com/BitBoxSwiss/bitbox02-api-go/util/errp"
2020
)
2121

22-
// queryBluetooth is like query, but nested one level deeper for Bluetooth.
23-
func (device *Device) queryBluetooth(request *messages.BluetoothRequest) (*messages.BluetoothResponse, error) {
22+
// nonAtomicQueryBluetooth is like query, but nested one level deeper for Bluetooth.
23+
func (device *Device) nonAtomicQueryBluetooth(request *messages.BluetoothRequest) (*messages.BluetoothResponse, error) {
2424
if !device.SupportsBluetooth() {
2525
return nil, errp.New("this device does not support Bluetooth")
2626
}
27-
response, err := device.query(&messages.Request{
27+
response, err := device.nonAtomicQuery(&messages.Request{
2828
Request: &messages.Request_Bluetooth{
2929
Bluetooth: request,
3030
},
@@ -41,44 +41,42 @@ func (device *Device) queryBluetooth(request *messages.BluetoothRequest) (*messa
4141

4242
// BluetoothUpgrade attempts an upgrade of the Bluetooth firmware.
4343
func (device *Device) BluetoothUpgrade(firmware []byte) error {
44-
// Send initial upgrade request
45-
initReq := &messages.BluetoothUpgradeInitRequest{
46-
FirmwareLength: uint32(len(firmware)),
47-
}
48-
req := &messages.BluetoothRequest{
49-
Request: &messages.BluetoothRequest_UpgradeInit{
50-
UpgradeInit: initReq,
51-
},
52-
}
53-
54-
currentResponse, err := device.queryBluetooth(req)
55-
if err != nil {
56-
return err
57-
}
44+
return device.atomicQueries(func() error {
45+
currentResponse, err := device.nonAtomicQueryBluetooth(&messages.BluetoothRequest{
46+
Request: &messages.BluetoothRequest_UpgradeInit{
47+
UpgradeInit: &messages.BluetoothUpgradeInitRequest{
48+
FirmwareLength: uint32(len(firmware)),
49+
},
50+
},
51+
})
52+
if err != nil {
53+
return err
54+
}
5855

59-
for {
60-
switch resp := currentResponse.Response.(type) {
61-
case *messages.BluetoothResponse_RequestChunk:
62-
chunkReq := resp.RequestChunk
63-
chunkData := firmware[chunkReq.Offset : chunkReq.Offset+chunkReq.Length]
56+
for {
57+
switch resp := currentResponse.Response.(type) {
58+
case *messages.BluetoothResponse_RequestChunk:
59+
chunkReq := resp.RequestChunk
60+
chunkData := firmware[chunkReq.Offset : chunkReq.Offset+chunkReq.Length]
6461

65-
currentResponse, err = device.queryBluetooth(&messages.BluetoothRequest{
66-
Request: &messages.BluetoothRequest_Chunk{
67-
Chunk: &messages.BluetoothChunkRequest{
68-
Data: chunkData,
62+
currentResponse, err = device.nonAtomicQueryBluetooth(&messages.BluetoothRequest{
63+
Request: &messages.BluetoothRequest_Chunk{
64+
Chunk: &messages.BluetoothChunkRequest{
65+
Data: chunkData,
66+
},
6967
},
70-
},
71-
})
72-
if err != nil {
73-
return err
74-
}
68+
})
69+
if err != nil {
70+
return err
71+
}
7572

76-
case *messages.BluetoothResponse_Success:
77-
// Upgrade complete
78-
return nil
73+
case *messages.BluetoothResponse_Success:
74+
// Upgrade complete
75+
return nil
7976

80-
default:
81-
return errp.New("unexpected response type during bluetooth upgrade")
77+
default:
78+
return errp.New("unexpected response type during bluetooth upgrade")
79+
}
8280
}
83-
}
81+
})
8482
}

‎api/firmware/btc.go

+57-23
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,9 @@ import (
3030

3131
const multisigNameMaxLen = 30
3232

33-
// queryBTC is like query, but nested one level deeper.
34-
func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) {
35-
response, err := device.query(&messages.Request{
33+
// nonAtomicQueryBTC is like nonAtomicQuery, but nested one level deeper.
34+
func (device *Device) nonAtomicQueryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) {
35+
response, err := device.nonAtomicQuery(&messages.Request{
3636
Request: &messages.Request_Btc{
3737
Btc: request,
3838
},
@@ -47,6 +47,13 @@ func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCRespo
4747
return btcResponse.Btc, nil
4848
}
4949

50+
// queryBTC is like query, but nested one level deeper.
51+
func (device *Device) queryBTC(request *messages.BTCRequest) (*messages.BTCResponse, error) {
52+
return atomicQueriesValue(device, func() (*messages.BTCResponse, error) {
53+
return device.nonAtomicQueryBTC(request)
54+
})
55+
}
56+
5057
// NewBTCScriptConfigSimple is a helper to construct the correct script config for simple script
5158
// types.
5259
func NewBTCScriptConfigSimple(typ messages.BTCScriptConfig_SimpleType) *messages.BTCScriptConfig {
@@ -172,9 +179,9 @@ func (device *Device) BTCAddress(
172179
return pubResponse.Pub.Pub, nil
173180
}
174181

175-
func (device *Device) queryBtcSign(request proto.Message) (
182+
func (device *Device) nonAtomicQueryBtcSign(request proto.Message) (
176183
*messages.BTCSignNextResponse, error) {
177-
response, err := device.query(request)
184+
response, err := device.nonAtomicQuery(request)
178185
if err != nil {
179186
return nil, err
180187
}
@@ -185,9 +192,9 @@ func (device *Device) queryBtcSign(request proto.Message) (
185192
return next.BtcSignNext, nil
186193
}
187194

188-
func (device *Device) nestedQueryBtcSign(request *messages.BTCRequest) (
195+
func (device *Device) nonAtomicNestedQueryBtcSign(request *messages.BTCRequest) (
189196
*messages.BTCSignNextResponse, error) {
190-
response, err := device.queryBTC(request)
197+
response, err := device.nonAtomicQueryBTC(request)
191198
if err != nil {
192199
return nil, err
193200
}
@@ -286,9 +293,7 @@ type BTCSignResult struct {
286293
GeneratedOutputs map[int][]byte
287294
}
288295

289-
// BTCSign signs a bitcoin or bitcoin-like transaction. The previous transactions of the inputs
290-
// need to be provided if `BTCSignNeedsPrevTxs()` returns true.
291-
func (device *Device) BTCSign(
296+
func (device *Device) nonAtomicBTCSign(
292297
coin messages.BTCCoin,
293298
scriptConfigs []*messages.BTCScriptConfigWithKeypath,
294299
outputScriptConfigs []*messages.BTCScriptConfigWithKeypath,
@@ -323,7 +328,7 @@ func (device *Device) BTCSign(
323328
}
324329

325330
signatures := make([][]byte, len(tx.Inputs))
326-
next, err := device.queryBtcSign(&messages.Request{
331+
next, err := device.nonAtomicQueryBtcSign(&messages.Request{
327332
Request: &messages.Request_BtcSignInit{
328333
BtcSignInit: &messages.BTCSignInitRequest{
329334
Coin: coin,
@@ -363,7 +368,7 @@ func (device *Device) BTCSign(
363368
Commitment: antikleptoHostCommit(hostNonce),
364369
}
365370
}
366-
next, err = device.queryBtcSign(&messages.Request{
371+
next, err = device.nonAtomicQueryBtcSign(&messages.Request{
367372
Request: &messages.Request_BtcSignInput{
368373
BtcSignInput: input,
369374
}})
@@ -376,7 +381,7 @@ func (device *Device) BTCSign(
376381
return nil, errp.New("unexpected response; expected signer nonce commitment")
377382
}
378383
signerCommitment := next.AntiKleptoSignerCommitment.Commitment
379-
next, err = device.nestedQueryBtcSign(
384+
next, err = device.nonAtomicNestedQueryBtcSign(
380385
&messages.BTCRequest{
381386
Request: &messages.BTCRequest_AntikleptoSignature{
382387
AntikleptoSignature: &messages.AntiKleptoSignatureRequest{
@@ -408,7 +413,7 @@ func (device *Device) BTCSign(
408413
}
409414
case messages.BTCSignNextResponse_PREVTX_INIT:
410415
prevtx := tx.Inputs[next.Index].PrevTx
411-
next, err = device.nestedQueryBtcSign(
416+
next, err = device.nonAtomicNestedQueryBtcSign(
412417
&messages.BTCRequest{
413418
Request: &messages.BTCRequest_PrevtxInit{
414419
PrevtxInit: &messages.BTCPrevTxInitRequest{
@@ -424,7 +429,7 @@ func (device *Device) BTCSign(
424429
}
425430
case messages.BTCSignNextResponse_PREVTX_INPUT:
426431
prevtxInput := tx.Inputs[next.Index].PrevTx.Inputs[next.PrevIndex]
427-
next, err = device.nestedQueryBtcSign(
432+
next, err = device.nonAtomicNestedQueryBtcSign(
428433
&messages.BTCRequest{
429434
Request: &messages.BTCRequest_PrevtxInput{
430435
PrevtxInput: prevtxInput,
@@ -435,7 +440,7 @@ func (device *Device) BTCSign(
435440
}
436441
case messages.BTCSignNextResponse_PREVTX_OUTPUT:
437442
prevtxOutput := tx.Inputs[next.Index].PrevTx.Outputs[next.PrevIndex]
438-
next, err = device.nestedQueryBtcSign(
443+
next, err = device.nonAtomicNestedQueryBtcSign(
439444
&messages.BTCRequest{
440445
Request: &messages.BTCRequest_PrevtxOutput{
441446
PrevtxOutput: prevtxOutput,
@@ -446,7 +451,7 @@ func (device *Device) BTCSign(
446451
}
447452
case messages.BTCSignNextResponse_OUTPUT:
448453
outputIndex := next.Index
449-
next, err = device.queryBtcSign(&messages.Request{
454+
next, err = device.nonAtomicQueryBtcSign(&messages.Request{
450455
Request: &messages.Request_BtcSignOutput{
451456
BtcSignOutput: tx.Outputs[outputIndex],
452457
}})
@@ -471,7 +476,7 @@ func (device *Device) BTCSign(
471476
return nil, errp.New("payment request index out of bounds")
472477
}
473478
paymentRequest := tx.PaymentRequests[paymentRequestIndex]
474-
next, err = device.nestedQueryBtcSign(
479+
next, err = device.nonAtomicNestedQueryBtcSign(
475480
&messages.BTCRequest{
476481
Request: &messages.BTCRequest_PaymentRequest{
477482
PaymentRequest: paymentRequest,
@@ -490,6 +495,26 @@ func (device *Device) BTCSign(
490495
}
491496
}
492497

498+
// BTCSign signs a bitcoin or bitcoin-like transaction. The previous transactions of the inputs
499+
// need to be provided if `BTCSignNeedsPrevTxs()` returns true.
500+
func (device *Device) BTCSign(
501+
coin messages.BTCCoin,
502+
scriptConfigs []*messages.BTCScriptConfigWithKeypath,
503+
outputScriptConfigs []*messages.BTCScriptConfigWithKeypath,
504+
tx *BTCTx,
505+
formatUnit messages.BTCSignInitRequest_FormatUnit,
506+
) (*BTCSignResult, error) {
507+
return atomicQueriesValue(device, func() (*BTCSignResult, error) {
508+
return device.nonAtomicBTCSign(
509+
coin,
510+
scriptConfigs,
511+
outputScriptConfigs,
512+
tx,
513+
formatUnit,
514+
)
515+
})
516+
}
517+
493518
// BTCIsScriptConfigRegistered returns true if the script config / account is already registered.
494519
func (device *Device) BTCIsScriptConfigRegistered(
495520
coin messages.BTCCoin,
@@ -562,9 +587,7 @@ type BTCSignMessageResult struct {
562587
ElectrumSig65 []byte
563588
}
564589

565-
// BTCSignMessage signs a Bitcoin message. The 64 byte raw signature, the recoverable ID and the 65
566-
// byte signature in Electrum format are returned.
567-
func (device *Device) BTCSignMessage(
590+
func (device *Device) nonAtomicBTCSignMessage(
568591
coin messages.BTCCoin,
569592
scriptConfig *messages.BTCScriptConfigWithKeypath,
570593
message []byte,
@@ -601,7 +624,7 @@ func (device *Device) BTCSignMessage(
601624
},
602625
},
603626
}
604-
response, err := device.queryBTC(request)
627+
response, err := device.nonAtomicQueryBTC(request)
605628
if err != nil {
606629
return nil, err
607630
}
@@ -612,7 +635,7 @@ func (device *Device) BTCSignMessage(
612635
if !ok {
613636
return nil, errp.New("unexpected response")
614637
}
615-
response, err := device.queryBTC(&messages.BTCRequest{
638+
response, err := device.nonAtomicQueryBTC(&messages.BTCRequest{
616639
Request: &messages.BTCRequest_AntikleptoSignature{
617640
AntikleptoSignature: &messages.AntiKleptoSignatureRequest{
618641
HostNonce: hostNonce,
@@ -654,3 +677,14 @@ func (device *Device) BTCSignMessage(
654677
ElectrumSig65: electrumSig65,
655678
}, nil
656679
}
680+
681+
// BTCSignMessage signs a Bitcoin message.
682+
func (device *Device) BTCSignMessage(
683+
coin messages.BTCCoin,
684+
scriptConfig *messages.BTCScriptConfigWithKeypath,
685+
message []byte,
686+
) (*BTCSignMessageResult, error) {
687+
return atomicQueriesValue(device, func() (*BTCSignMessageResult, error) {
688+
return device.nonAtomicBTCSignMessage(coin, scriptConfig, message)
689+
})
690+
}

‎api/firmware/device.go

+7
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,14 @@ type Device struct {
9494
// nonces must match the incrementing of the device nonces, so the decryption works. Avoid the
9595
// situation where we Encrypt() locally twice, and send the queries out of order, which will
9696
// result in a failed decryption on the device.
97+
//
98+
// Also, every request is followed by a response, so this lock ensures the responses is matched
99+
// to the request.
97100
queryLock sync.Mutex
101+
// While `queryLock` ensures that the response matches the request, `apiLock` ensures that a
102+
// series of request<>response pairs does not get interrupted. Some API calls require a series
103+
// of calls, like signing a BTC transactions.
104+
apiLock sync.Mutex
98105

99106
status Status
100107

‎api/firmware/query.go

+37-2
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import (
2727
)
2828

2929
const (
30-
// sinve v7.0.0, requets and responses are framed with hww* codes.
30+
// sinve v7.0.0, requests and responses are framed with hww* codes.
3131

3232
// hwwReq* are HWW-level framing opcodes of requests.
3333

@@ -130,7 +130,12 @@ func (device *Device) rawQuery(msg []byte) ([]byte, error) {
130130
return device.communication.Query(msg)
131131
}
132132

133-
func (device *Device) query(request proto.Message) (*messages.Response, error) {
133+
// Sends one request and returns the matching response.
134+
// This acquires the queryLock, so other queries can't interrupt and need to wait.
135+
// This *does not* acquire the apiLock.
136+
// Use this if you need to perform a series of requests that should not get interrupted.
137+
// Only use this inside `atomicQueries()`.
138+
func (device *Device) nonAtomicQuery(request proto.Message) (*messages.Response, error) {
134139
device.queryLock.Lock()
135140
defer device.queryLock.Unlock()
136141
if device.sendCipher == nil || !device.channelHashDeviceVerified || !device.channelHashAppVerified {
@@ -180,3 +185,33 @@ func (device *Device) query(request proto.Message) (*messages.Response, error) {
180185

181186
return response, nil
182187
}
188+
189+
// Sends one request and returns the matching response.
190+
// This acquires the queryLock, so other queries can't interrupt and need to wait.
191+
// This also acquires the apiLock, so series of request<>responses that should not be interrupted
192+
// don't get interrupted. Use `nonAtomicQuery()` if you need a series of request<>response pairs
193+
// that should not be interrupted.
194+
func (device *Device) query(request proto.Message) (*messages.Response, error) {
195+
device.apiLock.Lock()
196+
defer device.apiLock.Unlock()
197+
return device.nonAtomicQuery(request)
198+
}
199+
200+
// atomicQueries runs a series of device queries while holding the `apiLock` mutex, to ensure the
201+
// series of quries does not get interrupted.
202+
//
203+
// Important: only use `nonAtomicQuery` inside the callback (`query()` also acquires `apiLock` which
204+
// would lead to a deadlock).
205+
func (device *Device) atomicQueries(run func() error) error {
206+
device.apiLock.Lock()
207+
defer device.apiLock.Unlock()
208+
return run()
209+
}
210+
211+
// atomicQueriesValue is like `atomicQueries`, but allows returning a result value for convenience.
212+
// It is a function and not a method because Go does not support generic methods.
213+
func atomicQueriesValue[R any](device *Device, run func() (R, error)) (R, error) {
214+
device.apiLock.Lock()
215+
defer device.apiLock.Unlock()
216+
return run()
217+
}

0 commit comments

Comments
 (0)