| Message ID | 1d211023-3923-685b-20f0-f3f90ea56e1f@omp.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | of: module: prevent NULL pointer dereference in vsnprintf() | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Wed, Mar 27, 2024 at 9:52 AM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > > In of_modalias(), we can get passed the str and len parameters which would > cause a kernel oops in vsnprintf() since it only allows passing a NULL ptr > when the length is also 0. Also, we need to filter out the negative values > of the len parameter as these will result in a really huge buffer since > snprintf() takes size_t parameter while ours is ssize_t... > > Found by Linux Verification Center (linuxtesting.org) with the Svace static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > The patch is against the for-next branch of Rob Herring's linux-git repo... > > drivers/of/module.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > Index: linux/drivers/of/module.c > =================================================================== > --- linux.orig/drivers/of/module.c > +++ linux/drivers/of/module.c > @@ -16,6 +16,14 @@ ssize_t of_modalias(const struct device_ > ssize_t csize; > ssize_t tsize; > > + /* > + * Prevent a kernel oops in vsnprintf() -- it only allows passing a > + * NULL ptr when the length is also 0. Also filter out the negative > + * lengths... > + */ > + if ((len > 0 && !str) || len < 0) > + return -EINVAL; > + Even if vsnprintf() didn't oops, the input doesn't make sense. So, I think the comment should just say that. Or maybe just delete the comment since it'd be redundant at that point. Not a strong opinion. Acked-by: Saravana Kannan <saravanak@google.com> -Saravana > /* Name & Type */ > /* %p eats all alphanum characters, so %c must be used here */ > csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
On Wed, 27 Mar 2024 19:52:49 +0300, Sergey Shtylyov wrote: > In of_modalias(), we can get passed the str and len parameters which would > cause a kernel oops in vsnprintf() since it only allows passing a NULL ptr > when the length is also 0. Also, we need to filter out the negative values > of the len parameter as these will result in a really huge buffer since > snprintf() takes size_t parameter while ours is ssize_t... > > Found by Linux Verification Center (linuxtesting.org) with the Svace static > analysis tool. > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > The patch is against the for-next branch of Rob Herring's linux-git repo... > > drivers/of/module.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > Applied, thanks!
Index: linux/drivers/of/module.c =================================================================== --- linux.orig/drivers/of/module.c +++ linux/drivers/of/module.c @@ -16,6 +16,14 @@ ssize_t of_modalias(const struct device_ ssize_t csize; ssize_t tsize; + /* + * Prevent a kernel oops in vsnprintf() -- it only allows passing a + * NULL ptr when the length is also 0. Also filter out the negative + * lengths... + */ + if ((len > 0 && !str) || len < 0) + return -EINVAL; + /* Name & Type */ /* %p eats all alphanum characters, so %c must be used here */ csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
In of_modalias(), we can get passed the str and len parameters which would cause a kernel oops in vsnprintf() since it only allows passing a NULL ptr when the length is also 0. Also, we need to filter out the negative values of the len parameter as these will result in a really huge buffer since snprintf() takes size_t parameter while ours is ssize_t... Found by Linux Verification Center (linuxtesting.org) with the Svace static analysis tool. Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- The patch is against the for-next branch of Rob Herring's linux-git repo... drivers/of/module.c | 8 ++++++++ 1 file changed, 8 insertions(+)