Re: [PATCH net v2] nfc: llcp: fix slab-out-of-bounds read in nfc_llcp_wks_sap()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

- [Medium] Missing `Fixes:` tag on a [PATCH net] bug fix.

Pre-existing issues:
- [Medium] Prefix-match logic error in nfc_llcp_wks_sap()
  (net/nfc/llcp_core.c:351-357): the loop does `strncmp(wks[sap],…
- [Low] Unfixed sibling of the same defect class:
  nfc_llcp_build_sdreq_tlv() (net/nfc/llcp_commands.c:138) still does…

--

nfc: llcp: fix slab-out-of-bounds read in nfc_llcp_wks_sap()

This changes the pr_debug() in nfc_llcp_wks_sap() from %s to %.*s bounded
by service_name_len, so the non-NUL-terminated kmemdup()'d service name
copied in llcp_sock_bind() is no longer over-read by the printk format
scan.

Closes: https://syzkaller.appspot.com/bug?extid=1e3df0852e82c21ca418

> v2: Changed prefix to [PATCH net] but still I could not find the Fixes: tag both in the gitblame and the lore.

[Severity: Medium]
Regarding "still I could not find the Fixes: tag both in the gitblame and
the lore" - does git blame on the removed statement point at the original
LLCP support commit?

Blaming the pr_debug("%s\n", service_name); line in nfc_llcp_wks_sap() at
the baseline attributes it to:

  d646960f7986 ("NFC: Initial LLCP support")

Only the enclosing function signature was touched later, by 3df40eb3a2ea.
The unterminated kmemdup() of the user-supplied service name in
llcp_sock_bind() / llcp_sock_connect() comes from the same initial LLCP
series, so would:

  Fixes: d646960f7986 ("NFC: Initial LLCP support")

be the right tag here?  Without it the affected range (everything since
v3.3) has to be worked out by hand for stable backports, and the net tree
process asks for the tag on fixes.

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index cac1b5487064d..fda236e4d9fdc 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -341,7 +341,7 @@ static int nfc_llcp_wks_sap(const char *service_name, size_t service_name_len)
>  {
>  	int sap, num_wks;
>  
> -	pr_debug("%s\n", service_name);
> +	pr_debug("%.*s\n", (int)service_name_len, service_name);

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but is
there a second copy of the same pattern that could be converted in the
same change?  nfc_llcp_build_sdreq_tlv() in net/nfc/llcp_commands.c does:

  pr_debug("uri: %s, len: %zu\n", uri, uri_len);

The uri here is nla_data() of NFC_SDP_ATTR_URI, passed in from
nfc_genl_llc_sdreq() together with nla_len().  The policy entry is:

  [NFC_SDP_ATTR_URI] = { .type = NLA_STRING,
                         .len = U8_MAX - 4 },

NLA_STRING only trims an optional trailing NUL, it does not require one,
and the rest of nfc_llcp_build_sdreq_tlv() is written length-based
(if (uri[uri_len - 1] == 0) sdreq->tlv_len--; and memcpy(sdreq->uri, uri,
uri_len)), so no terminator is assumed there either.

Note this one does not appear to be able to produce the KASAN report the
changelog quotes: the payload sits in the netlink skb linear area and the
format scan stops on a zero byte inside the zeroed skb_shared_info of the
same allocation:

  net/core/skbuff.c:__alloc_skb() {
      ...
      memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
      ...
  }

What is left is the unbounded %s printing adjacent attribute bytes plus up
to three bytes of uninitialised SKB_DATA_ALIGN padding into dmesg, and it
needs CAP_NET_ADMIN (NFC_CMD_LLC_SDREQ carries GENL_ADMIN_PERM) plus an
enabled dyndbg callsite.  Would

  pr_debug("uri: %.*s, len: %zu\n", (int)uri_len, uri, uri_len);

be worth folding in for consistency?  The other %s users of a URI look
fine, since sdreq->uri points into a kzalloc(sdreq->tlv_len + 1).

>  
>  	if (service_name == NULL)
>  		return -EINVAL;
>  

The unquoted remainder of nfc_llcp_wks_sap() is the comparison loop:

  net/nfc/llcp_core.c:nfc_llcp_wks_sap() {
      ...
      for (sap = 0; sap < num_wks; sap++) {
              if (wks[sap] == NULL)
                      continue;

              if (strncmp(wks[sap], service_name, service_name_len) == 0)
                      return sap;
      }
      ...
  }

[Severity: Medium]
This is a pre-existing issue that the patch does not introduce, but since
the change lands two lines above it: should this comparison also require
service_name_len == strlen(wks[sap])?

As written, any strict prefix of a well-known name matches, for example
strncmp("urn:nfc:sn:ip", "urn:nfc:sn:i", 12) == 0 returns sap 2, and even a
single byte "u" matches.

nfc_llcp_wks_sap() is only reached from nfc_llcp_get_sdp_ssap(), which
passes the name and length verbatim from llcp_sock_bind() and
llcp_sock_connect().  Both just kmemdup() the user-supplied name under
min_t(..., NFC_LLCP_MAX_SERVICE_NAME) with no content validation, and
nfc_sock_create() does no capability check.

nfc_llcp_get_sdp_ssap() then treats the socket as the well-known service:

  net/nfc/llcp_core.c:nfc_llcp_get_sdp_ssap() {
      ...
      if (test_bit(ssap, &local->local_wks)) { ... return LLCP_SAP_MAX; }
      set_bit(ssap, &local->local_wks);
      return ssap;
      ...
  }

So a bind of "urn:nfc:sn:i" reserves the SAP belonging to
"urn:nfc:sn:ip", advertises it to the peer in the LLCP WKS general bytes,
and a later bind of the real name then fails with -EADDRINUSE.

The two peer-side sites in the same file do enforce exact length,
nfc_llcp_recv_snl() with:

  if (service_name_len == strlen("urn:nfc:sn:sdp") &&
      !strncmp(service_name, "urn:nfc:sn:sdp", service_name_len)) {

and nfc_llcp_sock_from_sn() skipping candidates whose service_name_len
differs before memcmp().  Is the missing length equality check here just an
oversight?  No out-of-bounds access is involved, strncmp reads at most
service_name_len bytes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905225211.596366-1-omermetekaya0%40gmail.com