laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-abis/+/28413 )

Change subject: osmo_pcap_lapd_write: Fix write of uninitialized byte(s) ......................................................................

osmo_pcap_lapd_write: Fix write of uninitialized byte(s)

The problem is that we don't zero-initialize the struct pcap_rechdr + pcap_lapdhdr before memcpy'ing them to buf, before we call write:

==20097== Syscall param write(buf) points to uninitialised byte(s) ==20097== at 0x4E48471: write (write.c:26) ==20097== by 0x4DA8DE9: osmo_pcap_lapd_write (lapd_pcap.c:168) ==20097== by 0x4DA8433: send_ph_data_req (lapd.c:628) ==20097== by 0x4C94F5C: lapd_send_rej (lapd_core.c:536) ==20097== by 0x4C9A08A: lapd_rx_i (lapd_core.c:1574) ==20097== by 0x4C9AA8F: lapd_ph_data_ind (lapd_core.c:1708) ==20097== by 0x4DA7C55: lapd_receive (lapd.c:496) ==20097== by 0x4D96B2C: e1inp_rx_ts_lapd (e1_input.c:778) ==20097== by 0x4D9C97C: handle_ts_sign_read (e1d.c:78) ==20097== by 0x4D9D908: e1d_fd_cb (e1d.c:281) ==20097== by 0x4D1281B: poll_disp_fds (select.c:361) ==20097== by 0x4D12928: _osmo_select_main (select.c:399) ==20097== Address 0x1ffefffed7 is on thread 1's stack ==20097== in frame #1, created by osmo_pcap_lapd_write (lapd_pcap.c:129)

The whole idea of first filling the two structs on the stack, and then copying them to another buffer on the stack is somehow weird. Let's just create a combined struct on the stack and then fill that one directly.

Change-Id: I358c71354cc6ddad1964cc4a988ad29b7ba617f1 Closes: OS#5592 --- M src/input/lapd_pcap.c 1 file changed, 22 insertions(+), 27 deletions(-)

Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve pespin: Looks good to me, approved

diff --git a/src/input/lapd_pcap.c b/src/input/lapd_pcap.c index 77d4971..6e1faf5 100644 --- a/src/input/lapd_pcap.c +++ b/src/input/lapd_pcap.c @@ -127,50 +127,45 @@ /* This currently only works for the D-Channel */ int osmo_pcap_lapd_write(int fd, int direction, struct msgb *msg) { - int numbytes = 0; struct timeval tv; - struct pcap_rechdr pcap_rechdr; - struct pcap_lapdhdr header; - char buf[sizeof(struct pcap_rechdr) + - sizeof(struct pcap_lapdhdr) + msg->len]; + struct { + struct pcap_rechdr pcap_rechdr; + struct pcap_lapdhdr header; + char buf[msg->len]; + } __attribute__((packed)) s;

/* PCAP file has not been opened, skip. */ if (fd < 0) return 0;

- pcap_rechdr.ts_sec = 0; - pcap_rechdr.ts_usec = 0; - pcap_rechdr.incl_len = msg->len + sizeof(struct pcap_lapdhdr); - pcap_rechdr.orig_len = msg->len + sizeof(struct pcap_lapdhdr); + memset(&s, 0, sizeof(s)); + + s.pcap_rechdr.ts_sec = 0; + s.pcap_rechdr.ts_usec = 0; + s.pcap_rechdr.incl_len = msg->len + sizeof(struct pcap_lapdhdr); + s.pcap_rechdr.orig_len = msg->len + sizeof(struct pcap_lapdhdr);

if (direction == OSMO_LAPD_PCAP_OUTPUT) - header.pkttype = htons(LINUX_SLL_OUTGOING); + s.header.pkttype = htons(LINUX_SLL_OUTGOING); else - header.pkttype = htons(LINUX_SLL_HOST); - header.hatype = 0; - header.halen = 0; - header.addr[0] = 0x01; /* we are the network side */ - header.protocol = ntohs(48); + s.header.pkttype = htons(LINUX_SLL_HOST); + s.header.hatype = 0; + s.header.halen = 0; + s.header.addr[0] = 0x01; /* we are the network side */ + s.header.protocol = ntohs(48);

gettimeofday(&tv, NULL); - pcap_rechdr.ts_sec = tv.tv_sec; - pcap_rechdr.ts_usec = tv.tv_usec; + s.pcap_rechdr.ts_sec = tv.tv_sec; + s.pcap_rechdr.ts_usec = tv.tv_usec;

- memcpy(buf + numbytes, &pcap_rechdr, sizeof(pcap_rechdr)); - numbytes += sizeof(pcap_rechdr); + memcpy(s.buf, msg->data, msg->len);

- memcpy(buf + numbytes, &header, sizeof(header)); - numbytes += sizeof(header); - - memcpy(buf + numbytes, msg->data, msg->len); - numbytes += msg->len; - - if (write(fd, buf, numbytes) != numbytes) { + if (write(fd, &s, sizeof(s)) != sizeof(s)) { LOGP(DLLAPD, LOGL_ERROR, "cannot write packet to PCAP: %s\n", strerror(errno)); return -1; } - return numbytes; + return sizeof(s); }

int osmo_pcap_lapd_close(int fd)