Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: ATT: Respond with not support error for unknown PDUs #6

Merged
merged 1 commit into from
May 2, 2017
Merged

Bluetooth: ATT: Respond with not support error for unknown PDUs #6

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions subsys/bluetooth/host/att.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
BT_GATT_PERM_WRITE_ENCRYPT)
#define BT_GATT_PERM_AUTHEN_MASK (BT_GATT_PERM_READ_AUTHEN | \
BT_GATT_PERM_WRITE_AUTHEN)
#define ATT_CMD_MASK 0x40

#define ATT_TIMEOUT K_SECONDS(30)

Expand Down Expand Up @@ -1825,6 +1826,10 @@ static att_type_t att_op_get_type(u8_t op)
}
}

if (op & ATT_CMD_MASK) {
return ATT_COMMAND;
}

return ATT_UNKNOWN;
}

Expand Down Expand Up @@ -1854,6 +1859,10 @@ static void bt_att_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)

if (!handler) {
BT_WARN("Unknown ATT code 0x%02x", hdr->code);
if (att_op_get_type(hdr->code) != ATT_COMMAND) {
send_err_rsp(chan->conn, hdr->code, 0,
BT_ATT_ERR_NOT_SUPPORTED);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that this is the right behavior. I'd have thought that unknown opcodes are essentially RFU, and any RFU fields are generally mandated to be ignored upon reception. So "not supported" is not quite the same as "unknown". Do you have some reference to the spec that says something about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how else we would handle new opcodes, if we just ignore we basically stall the request queue and no other request can be sent. Marcel had similar interpretation wich led us to change the behavior on BlueZ as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have some reference to a section in the spec that this interpretation comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F page 2179

If a server receives a request that it does not support, then the server shall
respond with the Error Response with the Error Code «Request Not
Supported», with the Attribute Handle In Error set to 0x0000.
If a server receives a command that it does not support, indicated by the
Command Flag of the PDU set to one, then the server shall ignore the
Command.

Note that the spec differentiate not supported request from commands, so only commands shall be ignored while request shall be replied. Now you can argue that request not supported is not exactly not understood, or something like that, still by not responding it will stall the request queue which inevitably will disconnect after 30 seconds and there is nowhere mentioned that an unknown PDU shall cause the server to disconnect. I don't know how likely it is to have any new requests introduced to ATT/GATT, but I guess it better to be safe than sorry if anything changes in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLUETOOTH SPECIFICATION Version 5.0 | Vol 3, Part F page 2173

A client may send attribute protocol requests to a server, and the server shall
respond to all requests that it receives.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it guaranteed that if the command flag is not set in an unrecognized PDU that it must then be a request PDU? There are more PDU types than requests and commands in ATT. Btw, I'm not saying that you're wrong (in that the patch would be better than not having it) but I still fail to see some unambiguous evidence for what's the right thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you'll want to rebase and reupload this one, since the rebase that was done for the bluetooth branch seems to have made github think this pull request contains multiple patches, when in reality it's just one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I will do this in a moment, regarding other types of PDUs honestly this seems an issue from the spec, though it is unlikely there will be new types of PDUs, other than notify and indicate. The spec also got inconsistent defining the notify and indicate PDUs, that should probably have its own mask like commands, instead the authors just jumped 0x1A and 0x1C.

return;
}

Expand Down