| Message ID | 1648182849-9209-1-git-send-email-baihaowen@meizu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: tcpm: testing array offset 'port->logbuffer_head' before use | expand |
On 3/24/22 21:34, Haowen Bai wrote: > Fix possible indexing array of bound for > port->logbuffer[port->logbuffer_head], where port->logbuffer_head boundary > check happens later. so we do it before. > > Signed-off-by: Haowen Bai <baihaowen@meizu.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 5fce795..541e9e4 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -591,6 +591,14 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args) > unsigned long rem_nsec; > > mutex_lock(&port->logbuffer_lock); > + > + if (port->logbuffer_head < 0 || > + port->logbuffer_head >= LOG_BUFFER_ENTRIES) { > + dev_warn(port->dev, > + "Bad log buffer index %d\n", port->logbuffer_head); > + goto abort; > + } > + > if (!port->logbuffer[port->logbuffer_head]) { > port->logbuffer[port->logbuffer_head] = > kzalloc(LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL); > @@ -607,13 +615,6 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args) > strcpy(tmpbuffer, "overflow"); > } > > - if (port->logbuffer_head < 0 || > - port->logbuffer_head >= LOG_BUFFER_ENTRIES) { > - dev_warn(port->dev, > - "Bad log buffer index %d\n", port->logbuffer_head); > - goto abort; > - } > - > if (!port->logbuffer[port->logbuffer_head]) { > dev_warn(port->dev, > "Log buffer index %d is NULL\n", port->logbuffer_head); One could argue that the check is unnecessary and can be removed as it can be proven that it logbuffer_head always in the range of [0, LOG_BUFFER_ENTRIES - 1]. Moving the check, however, does not add any value unless you can _prove_ that it needs to be moved, ie that logbuffer_head can be observed to be < 0 or >= LOG_BUFFER_ENTRIES. I think that is quite unlikely. Guenter
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 5fce795..541e9e4 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -591,6 +591,14 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args) unsigned long rem_nsec; mutex_lock(&port->logbuffer_lock); + + if (port->logbuffer_head < 0 || + port->logbuffer_head >= LOG_BUFFER_ENTRIES) { + dev_warn(port->dev, + "Bad log buffer index %d\n", port->logbuffer_head); + goto abort; + } + if (!port->logbuffer[port->logbuffer_head]) { port->logbuffer[port->logbuffer_head] = kzalloc(LOG_BUFFER_ENTRY_SIZE, GFP_KERNEL); @@ -607,13 +615,6 @@ static void _tcpm_log(struct tcpm_port *port, const char *fmt, va_list args) strcpy(tmpbuffer, "overflow"); } - if (port->logbuffer_head < 0 || - port->logbuffer_head >= LOG_BUFFER_ENTRIES) { - dev_warn(port->dev, - "Bad log buffer index %d\n", port->logbuffer_head); - goto abort; - } - if (!port->logbuffer[port->logbuffer_head]) { dev_warn(port->dev, "Log buffer index %d is NULL\n", port->logbuffer_head);
Fix possible indexing array of bound for port->logbuffer[port->logbuffer_head], where port->logbuffer_head boundary check happens later. so we do it before. Signed-off-by: Haowen Bai <baihaowen@meizu.com> --- drivers/usb/typec/tcpm/tcpm.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)