buglwIP - A Lightweight TCP/IP stack - Bugs: bug #62857, sendmsg(), if called with last...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #62857: sendmsg(), if called with last item in msg_iov array to have .iov_len=0, build pbuf chain which cannot be cloned

Submitter:  Alex Riesen <raalkml>
Submitted:  Wed 03 Aug 2022 11:03:40 AM UTC
   
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  None
Privacy:  Public Assigned to:  None
Open/Closed:  Open Planned Release:  None
lwIP version:  2.1.0

Thu 12 Oct 2023 12:31:40 PM UTC, comment #2: 

comment #1:

> From the description below, I'm tempted to change pbuf_clone() to just skip zero-sized pbufs (if anything has been copied yet). I guess that would be enough to fix this?


I think so. Maybe also change pbuf_clone to not ignore pbuf_copy errors? (Free the allocated pbuf and return NULL)

Alex Riesen <raalkml>
Wed 11 Oct 2023 08:25:38 PM UTC, comment #1: 

From the description below, I'm tempted to change pbuf_clone() to just skip zero-sized pbufs (if anything has been copied yet). I guess that would be enough to fix this?

Simon Goldschmidt <goldsimon>
Group administrator
Wed 03 Aug 2022 11:03:40 AM UTC, original submission:  

Hi,

I call lwip_sendmsg from LwIP 2.1.0 (release) with an UDP socket like this:


  static int udp_tx(int udp_skt, const void *PAYLOAD, size_t PAYLOAD_LENGTH)
  {
    struct iovec iov[2];
    struct msghdr msg;
    msg.msg_name = &remote_addr; /* global */
    msg.msg_namelen = sizeof(remote_addr);
    msg.msg_iov = iov;
    msg.msg_control = NULL;
    msg.msg_controllen = 0;
    msg.msg_flags = 0;
    iov[0].iov_base = &PROTOCOL_MAGIC;
    iov[0].iov_len = sizeof(PROTOCOL_MAGIC); /* 4 bytes */
    iov[1].iov_base = (void *)PAYLOAD;
    iov[1].iov_len = PAYLOAD_LENGTH;
    msg.msg_iovlen = 2;
    return lwip_sendmsg(udp_skt, &msg, 0);
  }


Usually, the payload is a string of bytes, but sometimes the function
"udp_tx" is called with zero-sized payload (end-of-transmission marker).

In those cases, the UDP packet on wire seems to contain random junk
instead of just whatever should be in the PROTOCOL_MAGIC.

After some debugging, it came to the low-level ethernet driver in my project,
which contains code like this:


  /* Check payload alignment in `p` */
  if ((uintptr_t)p->payload % MEM_ALIGNMENT != 0) {
    /* Create a single pbuf long enough to store the whole chain */
    struct pbuf *q = pbuf_clone(PBUF_RAW, PBUF_RAM, p);
    if (!q) ... error out ...
    p = q;
  } ...
  ... Pass p->payload to hardware ...


The Ethernet hardware apparently needs a contiguous, aligned data to transfer,
and the code tried to provide that.

The problem is that pbuf_clone returns not the expected pbuf containing the
flattened chain given in the "p" argument. The implementation in 2.1.0 (and if
I'm not mistaken, also in master) will return the allocated, partially filled
buffer, ignoring returned error from pbuf_copy!

The pbuf_copy in this case returns the errors because of this code:


    if ((p_from != NULL) && (p_from->len == p_from->tot_len)) {
      /* don't copy more than one packet! */
      LWIP_ERROR("pbuf_copy() does not allow packet queues!",
                 (p_from->next == NULL), return ERR_VAL;);
    }


On its final iterations, having processed Ethernet, IP, UDP, and the magic
from application, it reaches the zero-sized msg_iov item and returns with an
error code, which pbuf_clone mostly ignores (might print a debug message) and
returns new pbuf to the caller, which has no way to know that it is not the
clone it wanted.

Now, arguably "sendmsg" shouldn't have created such an evil chain of pbufs.
On the other hand, pbuf_clone is a public function and is even prepared to
fail (it returns NULL). On yet another hand, pbuf_copy may simply skip over
such buffers.

Right now, pbuf_clone just silently produces partial clones. And sendmsg silently
(under some circumstances, admitted) sends corrupted packets.


Alex Riesen <raalkml>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by raalkml (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    No changes have been made to this item

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code