| Message ID | 20230502183033.44496-3-jamestiotio@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | treewide: Fix multiple Coverity defects | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Wed, May 03, 2023 at 02:30:27AM +0800, James Raphael Tiovalen wrote: > This commit adds zero-initializations by changing `SFL_ALLOC` from > `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`, > initializing a `pollfd` struct variable with zeroes, and changing some > calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks > or undefined behavior from potentially uninitialized variables. > > Some variables would always be initialized by either the code flow or > the compiler. Thus, some of the associated Coverity reports might be > false positives. That said, it is still considered best practice to > zero-initialize variables upfront just in case to ensure the overall > resilience and security of OVS, as long as they do not impact > performance-critical code. As a bonus, it would also make static > analyzer tools, such as Coverity, happy. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Wed, May 03, 2023 at 02:30:27AM +0800, James Raphael Tiovalen wrote: > This commit adds zero-initializations by changing `SFL_ALLOC` from > `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`, > initializing a `pollfd` struct variable with zeroes, and changing some > calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks > or undefined behavior from potentially uninitialized variables. > > Some variables would always be initialized by either the code flow or > the compiler. Thus, some of the associated Coverity reports might be > false positives. That said, it is still considered best practice to > zero-initialize variables upfront just in case to ensure the overall > resilience and security of OVS, as long as they do not impact > performance-critical code. As a bonus, it would also make static > analyzer tools, such as Coverity, happy. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > --- > lib/latch-unix.c | 2 +- > lib/sflow_agent.c | 11 +++++++++-- > lib/sflow_api.h | 2 +- > utilities/ovs-vsctl.c | 5 ++--- > 4 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/lib/latch-unix.c b/lib/latch-unix.c > index f4d10c39a..c62bb024b 100644 > --- a/lib/latch-unix.c > +++ b/lib/latch-unix.c > @@ -71,7 +71,7 @@ latch_set(struct latch *latch) > bool > latch_is_set(const struct latch *latch) > { > - struct pollfd pfd; > + struct pollfd pfd = {0}; > int retval; > > pfd.fd = latch->fds[0]; > diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c > index c95f654a5..162e3c3f4 100644 > --- a/lib/sflow_agent.c > +++ b/lib/sflow_agent.c > @@ -510,8 +510,15 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, char *msg) > > static void * sflAlloc(SFLAgent *agent, size_t bytes) > { > - if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes); > - else return SFL_ALLOC(bytes); > + void *alloc; > + if (agent->allocFn) { > + alloc = (*agent->allocFn)(agent->magic, agent, bytes); > + memset(alloc, 0, bytes); > + } else { > + alloc = SFL_ALLOC(bytes); > + } > + ovs_assert(alloc); > + return alloc; > } On question about this. memset() is passed alloc before the call to ovs_assert(). From the POV of the safety you are implementing, is that ok?
On Wed, May 24, 2023 at 12:20 AM Simon Horman <simon.horman@corigine.com> wrote: > > On question about this. > > memset() is passed alloc before the call to ovs_assert(). > From the POV of the safety you are implementing, is that ok? Ah right, apologies for that. It seems that I overlooked the order. `ovs_assert` should be called before `memset` since calling `memset` on a null pointer results in undefined behavior. I will correct this in the next patch version. Best regards, James Raphael Tiovalen
diff --git a/lib/latch-unix.c b/lib/latch-unix.c index f4d10c39a..c62bb024b 100644 --- a/lib/latch-unix.c +++ b/lib/latch-unix.c @@ -71,7 +71,7 @@ latch_set(struct latch *latch) bool latch_is_set(const struct latch *latch) { - struct pollfd pfd; + struct pollfd pfd = {0}; int retval; pfd.fd = latch->fds[0]; diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c index c95f654a5..162e3c3f4 100644 --- a/lib/sflow_agent.c +++ b/lib/sflow_agent.c @@ -510,8 +510,15 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, char *msg) static void * sflAlloc(SFLAgent *agent, size_t bytes) { - if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes); - else return SFL_ALLOC(bytes); + void *alloc; + if (agent->allocFn) { + alloc = (*agent->allocFn)(agent->magic, agent, bytes); + memset(alloc, 0, bytes); + } else { + alloc = SFL_ALLOC(bytes); + } + ovs_assert(alloc); + return alloc; } static void sflFree(SFLAgent *agent, void *obj) diff --git a/lib/sflow_api.h b/lib/sflow_api.h index a0530b37a..eb23e2acd 100644 --- a/lib/sflow_api.h +++ b/lib/sflow_api.h @@ -337,7 +337,7 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, char *msg); u_int32_t sfl_receiver_samplePacketsSent(SFLReceiver *receiver); -#define SFL_ALLOC malloc +#define SFL_ALLOC xzalloc #define SFL_FREE free #endif /* SFLOW_API_H */ diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c index 2f5ac1a26..b3c75f8ba 100644 --- a/utilities/ovs-vsctl.c +++ b/utilities/ovs-vsctl.c @@ -575,7 +575,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx, struct ovsrec_bridge *br_cfg, const char *name, struct vsctl_bridge *parent, int vlan) { - struct vsctl_bridge *br = xmalloc(sizeof *br); + struct vsctl_bridge *br = xzalloc(sizeof *br); br->br_cfg = br_cfg; br->name = xstrdup(name); ovs_list_init(&br->ports); @@ -659,7 +659,7 @@ static struct vsctl_port * add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, struct ovsrec_port *port_cfg) { - struct vsctl_port *port; + struct vsctl_port *port = xzalloc(sizeof *port); if (port_cfg->tag && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) { @@ -671,7 +671,6 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge *parent, } } - port = xmalloc(sizeof *port); ovs_list_push_back(&parent->ports, &port->ports_node); ovs_list_init(&port->ifaces); port->port_cfg = port_cfg;
This commit adds zero-initializations by changing `SFL_ALLOC` from `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`, initializing a `pollfd` struct variable with zeroes, and changing some calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks or undefined behavior from potentially uninitialized variables. Some variables would always be initialized by either the code flow or the compiler. Thus, some of the associated Coverity reports might be false positives. That said, it is still considered best practice to zero-initialize variables upfront just in case to ensure the overall resilience and security of OVS, as long as they do not impact performance-critical code. As a bonus, it would also make static analyzer tools, such as Coverity, happy. Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> --- lib/latch-unix.c | 2 +- lib/sflow_agent.c | 11 +++++++++-- lib/sflow_api.h | 2 +- utilities/ovs-vsctl.c | 5 ++--- 4 files changed, 13 insertions(+), 7 deletions(-)