On Wed, 3 Jul 2019 at 02:42, Zhang Chen <chen.zh...@intel.com> wrote: > > From: Zhang Chen <chen.zh...@intel.com> > > Address Peter's comments in patch "COLO-compare:Add colo-compare > remote notify support". > > Signed-off-by: Zhang Chen <chen.zh...@intel.com> > --- > net/colo-compare.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c > index 909dd6c6eb..363b1edd11 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -1008,21 +1008,20 @@ static void > compare_notify_rs_finalize(SocketReadState *notify_rs) > { > CompareState *s = container_of(notify_rs, CompareState, notify_rs); > > - /* Get Xen colo-frame's notify and handle the message */ > - char *data = g_memdup(notify_rs->buf, notify_rs->packet_len); > - char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > + const char msg[] = "COLO_COMPARE_GET_XEN_INIT"; > int ret; > > - if (!strcmp(data, "COLO_USERSPACE_PROXY_INIT")) { > + if (!strcmp((char *)notify_rs->buf, "COLO_USERSPACE_PROXY_INIT")) {
This is (still) assuming that the buffer you're passed in has a NUL-terminated string: if not, it could run off the end of it. What you want to check is: (1) is the packet_len long enough for the string we're looking for (including the terminating NUL) and (2) if so, does a simple "compare these N bytes" check match? Something like static bool packet_matches_str(const char *str, uint8_t *buf, uint32_t packet_len) { if (packet_len <= strlen(str)) { return false; } return !memcmp(str, buf, strlen(str) + 1); } might be a useful utility function. (notice that we are including the NUL byte in our comparison check). In general this code doesn't seem to have been written with an eye to the packet contents being possibly-malicious. For instance colo_compare_packet_payload() doesn't seem to check that the packets actually are both long enough for the length being compared. This could perhaps do with some review/audit by somebody. thanks -- PMM