| Message ID | 20201209030716.3764-1-ruc_zhangxiaohui@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] tcpm: Fix possible buffer overflows in tcpm_queue_vdm | expand |
Hi, On Wed, Dec 09, 2020 at 11:07:16AM +0800, Xiaohui Zhang wrote: > From: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > > tcpm_queue_vdm() calls memcpy() without checking the destination > size may trigger a buffer overflower. Thanks for the patch, but I didn't actually see any place where that could happen. I think the idea is that the callers make sure the count does not exceed VDO_MAX_SIZE before calling the function. > Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 55535c4f6..fcd331f33 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -1045,7 +1045,7 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, > > port->vdo_count = cnt + 1; That should have been fixed as well, no? > port->vdo_data[0] = header; > - memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt); > + memcpy(&port->vdo_data[1], data, min_t(int, sizeof(u32) * cnt, VDO_MAX_SIZE - 1)); > /* Set ready, vdm state machine will actually send */ > port->vdm_retries = 0; > port->vdm_state = VDM_STATE_READY; Unless I'm missing something, I don't think this patch is needed. thanks, -- heikki
On 12/11/20 2:09 AM, Heikki Krogerus wrote: > Hi, > > On Wed, Dec 09, 2020 at 11:07:16AM +0800, Xiaohui Zhang wrote: >> From: Zhang Xiaohui <ruc_zhangxiaohui@163.com> >> >> tcpm_queue_vdm() calls memcpy() without checking the destination >> size may trigger a buffer overflower. > > Thanks for the patch, but I didn't actually see any place where that > could happen. I think the idea is that the callers make sure the count > does not exceed VDO_MAX_SIZE before calling the function. > Yes, when I wrote the code I made sure that this is the case. If that is no longer true, we have various other problems because the count is assumed to be in range pretty much everywhere. Guenter >> Signed-off-by: Zhang Xiaohui <ruc_zhangxiaohui@163.com> >> --- >> drivers/usb/typec/tcpm/tcpm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c >> index 55535c4f6..fcd331f33 100644 >> --- a/drivers/usb/typec/tcpm/tcpm.c >> +++ b/drivers/usb/typec/tcpm/tcpm.c >> @@ -1045,7 +1045,7 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, >> >> port->vdo_count = cnt + 1; > > That should have been fixed as well, no? > >> port->vdo_data[0] = header; >> - memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt); >> + memcpy(&port->vdo_data[1], data, min_t(int, sizeof(u32) * cnt, VDO_MAX_SIZE - 1)); >> /* Set ready, vdm state machine will actually send */ >> port->vdm_retries = 0; >> port->vdm_state = VDM_STATE_READY; > > Unless I'm missing something, I don't think this patch is needed. > > thanks, >
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 55535c4f6..fcd331f33 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -1045,7 +1045,7 @@ static void tcpm_queue_vdm(struct tcpm_port *port, const u32 header, port->vdo_count = cnt + 1; port->vdo_data[0] = header; - memcpy(&port->vdo_data[1], data, sizeof(u32) * cnt); + memcpy(&port->vdo_data[1], data, min_t(int, sizeof(u32) * cnt, VDO_MAX_SIZE - 1)); /* Set ready, vdm state machine will actually send */ port->vdm_retries = 0; port->vdm_state = VDM_STATE_READY;