| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201130184845.304f54d3@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date: Mon, 30 Nov 2020 18:48:45 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: bongsu.jeon2@...il.com
Cc: linux-nfc@...ts.01.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, Bongsu Jeon <bongsu.jeon@...sung.com>
Subject: Re: [PATCH net-next v3] net/nfc/nci: Support NCI 2.x initial
sequence
On Fri, 27 Nov 2020 22:36:31 +0900 bongsu.jeon2@...il.com wrote:
> From: Bongsu Jeon <bongsu.jeon@...sung.com>
>
> implement the NCI 2.x initial sequence to support NCI 2.x NFCC.
> Since NCI 2.0, CORE_RESET and CORE_INIT sequence have been changed.
> If NFCEE supports NCI 2.x, then NCI 2.x initial sequence will work.
>
> In NCI 1.0, Initial sequence and payloads are as below:
> (DH) (NFCC)
> | -- CORE_RESET_CMD --> |
> | <-- CORE_RESET_RSP -- |
> | -- CORE_INIT_CMD --> |
> | <-- CORE_INIT_RSP -- |
> CORE_RESET_RSP payloads are Status, NCI version, Configuration Status.
> CORE_INIT_CMD payloads are empty.
> CORE_INIT_RSP payloads are Status, NFCC Features,
> Number of Supported RF Interfaces, Supported RF Interface,
> Max Logical Connections, Max Routing table Size,
> Max Control Packet Payload Size, Max Size for Large Parameters,
> Manufacturer ID, Manufacturer Specific Information.
>
> In NCI 2.0, Initial Sequence and Parameters are as below:
> (DH) (NFCC)
> | -- CORE_RESET_CMD --> |
> | <-- CORE_RESET_RSP -- |
> | <-- CORE_RESET_NTF -- |
> | -- CORE_INIT_CMD --> |
> | <-- CORE_INIT_RSP -- |
> CORE_RESET_RSP payloads are Status.
> CORE_RESET_NTF payloads are Reset Trigger,
> Configuration Status, NCI Version, Manufacturer ID,
> Manufacturer Specific Information Length,
> Manufacturer Specific Information.
> CORE_INIT_CMD payloads are Feature1, Feature2.
> CORE_INIT_RSP payloads are Status, NFCC Features,
> Max Logical Connections, Max Routing Table Size,
> Max Control Packet Payload Size,
> Max Data Packet Payload Size of the Static HCI Connection,
> Number of Credits of the Static HCI Connection,
> Max NFC-V RF Frame Size, Number of Supported RF Interfaces,
> Supported RF Interfaces.
>
> Signed-off-by: Bongsu Jeon <bongsu.jeon@...sung.com>
> static void nci_init_req(struct nci_dev *ndev, unsigned long opt)
> {
> - nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL);
> + struct nci_core_init_v2_cmd *cmd = (struct nci_core_init_v2_cmd *)opt;
> +
> + if (!cmd)
> + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, 0, NULL);
> + else
> + /* if nci version is 2.0, then use the feature parameters */
> + nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD,
> + sizeof(struct nci_core_init_v2_cmd), cmd);
This would be better written as:
u8 plen = 0;
if (opt)
plen = sizeof(struct nci_core_init_v2_cmd);
nci_send_cmd(ndev, NCI_OP_CORE_INIT_CMD, plen, (void *)opt);
> +
unnecessary empty line
> }
>
> static void nci_init_complete_req(struct nci_dev *ndev, unsigned long opt)
> @@ -497,8 +505,18 @@ static int nci_open_device(struct nci_dev *ndev)
> }
>
> if (!rc) {
> - rc = __nci_request(ndev, nci_init_req, 0,
> - msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + if (!(ndev->nci_ver & NCI_VER_2_MASK)) {
> + rc = __nci_request(ndev, nci_init_req, 0,
> + msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + } else {
> + struct nci_core_init_v2_cmd nci_init_v2_cmd;
> +
> + nci_init_v2_cmd.feature1 = NCI_FEATURE_DISABLE;
> + nci_init_v2_cmd.feature2 = NCI_FEATURE_DISABLE;
> +
> + rc = __nci_request(ndev, nci_init_req, (unsigned long)&nci_init_v2_cmd,
> + msecs_to_jiffies(NCI_INIT_TIMEOUT));
> + }
again please try to pull out the common code:
struct nci_core_init_v2_cmd nci_init_v2_cmd = {
.feature1 = NCI_FEATURE_DISABLE;
.feature2 = NCI_FEATURE_DISABLE;
};
unsigned long opt = 0;
if (ndev->nci_ver & NCI_VER_2_MASK)
opt = (unsigned long)&nci_init_v2_cmd;
rc = __nci_request(ndev, nci_init_req, opt,
msecs_to_jiffies(NCI_INIT_TIMEOUT));
> }
> -static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb)
> +static unsigned char nci_core_init_rsp_packet_v1(struct nci_dev *ndev, struct sk_buff *skb)
> {
> struct nci_core_init_rsp_1 *rsp_1 = (void *) skb->data;
> struct nci_core_init_rsp_2 *rsp_2;
> @@ -48,16 +51,14 @@ static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb)
> pr_debug("status 0x%x\n", rsp_1->status);
>
> if (rsp_1->status != NCI_STATUS_OK)
> - goto exit;
> + return rsp_1->status;
>
> ndev->nfcc_features = __le32_to_cpu(rsp_1->nfcc_features);
> ndev->num_supported_rf_interfaces = rsp_1->num_supported_rf_interfaces;
>
> - if (ndev->num_supported_rf_interfaces >
> - NCI_MAX_SUPPORTED_RF_INTERFACES) {
> - ndev->num_supported_rf_interfaces =
> - NCI_MAX_SUPPORTED_RF_INTERFACES;
> - }
> + ndev->num_supported_rf_interfaces =
> + min((int)ndev->num_supported_rf_interfaces,
> + NCI_MAX_SUPPORTED_RF_INTERFACES);
>
> memcpy(ndev->supported_rf_interfaces,
> rsp_1->supported_rf_interfaces,
> @@ -77,6 +78,58 @@ static void nci_core_init_rsp_packet(struct nci_dev *ndev, struct sk_buff *skb)
> ndev->manufact_specific_info =
> __le32_to_cpu(rsp_2->manufact_specific_info);
>
> + return NCI_STATUS_OK;
> +}
> +
> +static unsigned char nci_core_init_rsp_packet_v2(struct nci_dev *ndev, struct sk_buff *skb)
> +{
> + struct nci_core_init_rsp_nci_ver2 *rsp = (void *)skb->data;
> + unsigned char rf_interface_idx = 0;
Prefer the use of u8 type in the kernel
> + unsigned char rf_extension_cnt = 0;
> + unsigned char *supported_rf_interface = rsp->supported_rf_interfaces;
Please order the variable declarations longest to shortest.
Don't initialize them inline if that'd cause the order to break.
> + pr_debug("status %x\n", rsp->status);
> +
> + if (rsp->status != NCI_STATUS_OK)
> + return rsp->status;
> +
Powered by blists - more mailing lists