>From 2b1ee0de3af88b5f85c451906a8854fc7b9c21ae Mon Sep 17 00:00:00 2001 From: David Lockyer Date: Mon, 11 Sep 2017 10:29:41 +0100 Subject: [PATCH] Fixed a memory access fault in lwip_select(), when using MPU. --- src/api/sockets.c | 85 +++++++++++++++++----------------------- src/include/lwip/opt.h | 8 ++++ src/include/lwip/priv/memp_std.h | 4 ++ src/include/lwip/sockets.h | 27 +++++++++++++ 4 files changed, 74 insertions(+), 50 deletions(-) diff --git a/src/api/sockets.c b/src/api/sockets.c index b763248..83812b1 100644 --- a/src/api/sockets.c +++ b/src/api/sockets.c @@ -50,7 +50,6 @@ #include "lwip/sockets.h" #include "lwip/api.h" -#include "lwip/sys.h" #include "lwip/igmp.h" #include "lwip/inet.h" #include "lwip/tcp.h" @@ -77,6 +76,13 @@ #define LWIP_NETCONN 0 #endif +#define API_SELECT_CB_VAR_REF(name) API_VAR_REF(name) +#define API_SELECT_CB_VAR_DECLARE(name) API_VAR_DECLARE(struct lwip_select_cb, name) +#define API_SELECT_CB_VAR_ALLOC(name) API_VAR_ALLOC(struct lwip_select_cb, MEMP_SELECT_CB, name, ERR_MEM) +#define API_SELECT_CB_VAR_ALLOC_RETURN_NULL(name) API_VAR_ALLOC(struct lwip_select_cb, MEMP_SELECT_CB, name, NULL) +#define API_SELECT_CB_VAR_FREE(name) API_VAR_FREE(MEMP_SELECT_CB, name) + + #if LWIP_IPV4 #define IP4ADDR_PORT_TO_SOCKADDR(sin, ipaddr, port) do { \ (sin)->sin_len = sizeof(struct sockaddr_in); \ @@ -218,32 +224,6 @@ struct lwip_sock { SELWAIT_T select_waiting; }; -#if LWIP_NETCONN_SEM_PER_THREAD -#define SELECT_SEM_T sys_sem_t* -#define SELECT_SEM_PTR(sem) (sem) -#else /* LWIP_NETCONN_SEM_PER_THREAD */ -#define SELECT_SEM_T sys_sem_t -#define SELECT_SEM_PTR(sem) (&(sem)) -#endif /* LWIP_NETCONN_SEM_PER_THREAD */ - -/** Description for a task waiting in select */ -struct lwip_select_cb { - /** Pointer to the next waiting task */ - struct lwip_select_cb *next; - /** Pointer to the previous waiting task */ - struct lwip_select_cb *prev; - /** readset passed to select */ - fd_set *readset; - /** writeset passed to select */ - fd_set *writeset; - /** unimplemented: exceptset passed to select */ - fd_set *exceptset; - /** don't signal the same semaphore twice: set to 1 when signalled */ - int sem_signalled; - /** semaphore to wake up a task waiting for select */ - SELECT_SEM_T sem; -}; - /** A struct sockaddr replacement that has the same alignment as sockaddr_in/ * sockaddr_in6 if instantiated. */ @@ -1375,7 +1355,7 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, int nready; fd_set lreadset, lwriteset, lexceptset; u32_t msectimeout; - struct lwip_select_cb select_cb; + API_SELECT_CB_VAR_DECLARE(select_cb); int i; int maxfdp2; #if LWIP_NETCONN_SEM_PER_THREAD @@ -1383,6 +1363,8 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, #endif SYS_ARCH_DECL_PROTECT(lev); + API_SELECT_CB_VAR_ALLOC(select_cb); + LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_select(%d, %p, %p, %p, tvsec=%"S32_F" tvusec=%"S32_F")\n", maxfdp1, (void *)readset, (void *) writeset, (void *) exceptset, timeout ? (s32_t)timeout->tv_sec : (s32_t)-1, @@ -1406,18 +1388,19 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, list is only valid while we are in this function, so it's ok to use local variables. */ - select_cb.next = NULL; - select_cb.prev = NULL; - select_cb.readset = readset; - select_cb.writeset = writeset; - select_cb.exceptset = exceptset; - select_cb.sem_signalled = 0; + API_SELECT_CB_VAR_REF(select_cb).next = NULL; + API_SELECT_CB_VAR_REF(select_cb).prev = NULL; + API_SELECT_CB_VAR_REF(select_cb).readset = readset; + API_SELECT_CB_VAR_REF(select_cb).writeset = writeset; + API_SELECT_CB_VAR_REF(select_cb).exceptset = exceptset; + API_SELECT_CB_VAR_REF(select_cb).sem_signalled = 0; #if LWIP_NETCONN_SEM_PER_THREAD - select_cb.sem = LWIP_NETCONN_THREAD_SEM_GET(); + API_SELECT_CB_VAR_REF(select_cb).sem = LWIP_NETCONN_THREAD_SEM_GET(); #else /* LWIP_NETCONN_SEM_PER_THREAD */ - if (sys_sem_new(&select_cb.sem, 0) != ERR_OK) { + if (sys_sem_new(&API_SELECT_CB_VAR_REF(select_cb).sem, 0) != ERR_OK) { /* failed to create semaphore */ set_errno(ENOMEM); + API_SELECT_CB_VAR_FREE(select_cb); return -1; } #endif /* LWIP_NETCONN_SEM_PER_THREAD */ @@ -1426,11 +1409,11 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, SYS_ARCH_PROTECT(lev); /* Put this select_cb on top of list */ - select_cb.next = select_cb_list; + API_SELECT_CB_VAR_REF(select_cb).next = select_cb_list; if (select_cb_list != NULL) { - select_cb_list->prev = &select_cb; + select_cb_list->prev = &API_SELECT_CB_VAR_REF(select_cb); } - select_cb_list = &select_cb; + select_cb_list = &API_SELECT_CB_VAR_REF(select_cb); /* Increasing this counter tells event_callback that the list has changed. */ select_cb_ctr++; @@ -1477,7 +1460,7 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, } } - waitres = sys_arch_sem_wait(SELECT_SEM_PTR(select_cb.sem), msectimeout); + waitres = sys_arch_sem_wait(SELECT_SEM_PTR(API_SELECT_CB_VAR_REF(select_cb).sem), msectimeout); #if LWIP_NETCONN_SEM_PER_THREAD waited = 1; #endif @@ -1507,32 +1490,33 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset, } /* Take us off the list */ SYS_ARCH_PROTECT(lev); - if (select_cb.next != NULL) { - select_cb.next->prev = select_cb.prev; + if (API_SELECT_CB_VAR_REF(select_cb).next != NULL) { + API_SELECT_CB_VAR_REF(select_cb).next->prev = API_SELECT_CB_VAR_REF(select_cb).prev; } - if (select_cb_list == &select_cb) { - LWIP_ASSERT("select_cb.prev == NULL", select_cb.prev == NULL); - select_cb_list = select_cb.next; + if (select_cb_list == &API_SELECT_CB_VAR_REF(select_cb)) { + LWIP_ASSERT("select_cb.prev == NULL", API_SELECT_CB_VAR_REF(select_cb).prev == NULL); + select_cb_list = API_SELECT_CB_VAR_REF(select_cb).next; } else { - LWIP_ASSERT("select_cb.prev != NULL", select_cb.prev != NULL); - select_cb.prev->next = select_cb.next; + LWIP_ASSERT("select_cb.prev != NULL", API_SELECT_CB_VAR_REF(select_cb).prev != NULL); + API_SELECT_CB_VAR_REF(select_cb).prev->next = API_SELECT_CB_VAR_REF(select_cb).next; } /* Increasing this counter tells event_callback that the list has changed. */ select_cb_ctr++; SYS_ARCH_UNPROTECT(lev); #if LWIP_NETCONN_SEM_PER_THREAD - if (select_cb.sem_signalled && (!waited || (waitres == SYS_ARCH_TIMEOUT))) { + if (API_SELECT_CB_VAR_REF(select_cb).sem_signalled && (!waited || (waitres == SYS_ARCH_TIMEOUT))) { /* don't leave the thread-local semaphore signalled */ - sys_arch_sem_wait(select_cb.sem, 1); + sys_arch_sem_wait(API_SELECT_CB_VAR_REF(select_cb).sem, 1); } #else /* LWIP_NETCONN_SEM_PER_THREAD */ - sys_sem_free(&select_cb.sem); + sys_sem_free(&API_SELECT_CB_VAR_REF(select_cb).sem); #endif /* LWIP_NETCONN_SEM_PER_THREAD */ if (nready < 0) { /* This happens when a socket got closed while waiting */ set_errno(EBADF); + API_SELECT_CB_VAR_FREE(select_cb); return -1; } @@ -1560,6 +1544,7 @@ return_copy_fdsets: if (exceptset) { *exceptset = lexceptset; } + API_SELECT_CB_VAR_FREE(select_cb); return nready; } diff --git a/src/include/lwip/opt.h b/src/include/lwip/opt.h index fd459af..01a09f9 100644 --- a/src/include/lwip/opt.h +++ b/src/include/lwip/opt.h @@ -455,6 +455,14 @@ #endif /** + * MEMP_NUM_SELECT_CB: the number of struct lwip_select_cb. + * (only needed if you use the sequential API, like api_lib.c) + */ +#if !defined MEMP_NUM_SELECT_CB || defined __DOXYGEN__ +#define MEMP_NUM_SELECT_CB 4 +#endif + +/** * MEMP_NUM_TCPIP_MSG_API: the number of struct tcpip_msg, which are used * for callback/timeout API communication. * (only needed if you use tcpip.c) diff --git a/src/include/lwip/priv/memp_std.h b/src/include/lwip/priv/memp_std.h index ce9fd50..144d965 100644 --- a/src/include/lwip/priv/memp_std.h +++ b/src/include/lwip/priv/memp_std.h @@ -64,6 +64,10 @@ LWIP_MEMPOOL(NETBUF, MEMP_NUM_NETBUF, sizeof(struct netbuf), LWIP_MEMPOOL(NETCONN, MEMP_NUM_NETCONN, sizeof(struct netconn), "NETCONN") #endif /* LWIP_NETCONN || LWIP_SOCKET */ +#if LWIP_SOCKET +LWIP_MEMPOOL(SELECT_CB, MEMP_NUM_SELECT_CB, sizeof(struct lwip_select_cb), "SELECT_CB") +#endif /* LWIP_SOCKET */ + #if NO_SYS==0 LWIP_MEMPOOL(TCPIP_MSG_API, MEMP_NUM_TCPIP_MSG_API, sizeof(struct tcpip_msg), "TCPIP_MSG_API") #if LWIP_MPU_COMPATIBLE diff --git a/src/include/lwip/sockets.h b/src/include/lwip/sockets.h index 2522056..0112fbb 100644 --- a/src/include/lwip/sockets.h +++ b/src/include/lwip/sockets.h @@ -47,6 +47,7 @@ #include "lwip/err.h" #include "lwip/inet.h" #include "lwip/errno.h" +#include "lwip/sys.h" #ifdef __cplusplus extern "C" { @@ -159,6 +160,32 @@ struct msghdr { int msg_flags; }; +#if LWIP_NETCONN_SEM_PER_THREAD +#define SELECT_SEM_T sys_sem_t* +#define SELECT_SEM_PTR(sem) (sem) +#else /* LWIP_NETCONN_SEM_PER_THREAD */ +#define SELECT_SEM_T sys_sem_t +#define SELECT_SEM_PTR(sem) (&(sem)) +#endif /* LWIP_NETCONN_SEM_PER_THREAD */ + +/** Description for a task waiting in select */ +struct lwip_select_cb { + /** Pointer to the next waiting task */ + struct lwip_select_cb *next; + /** Pointer to the previous waiting task */ + struct lwip_select_cb *prev; + /** readset passed to select */ + fd_set *readset; + /** writeset passed to select */ + fd_set *writeset; + /** unimplemented: exceptset passed to select */ + fd_set *exceptset; + /** don't signal the same semaphore twice: set to 1 when signalled */ + int sem_signalled; + /** semaphore to wake up a task waiting for select */ + SELECT_SEM_T sem; +}; + /* Socket protocol types (TCP/UDP/RAW) */ #define SOCK_STREAM 1 #define SOCK_DGRAM 2 -- 2.7.4