| Message ID | 20210608062923.94017-2-ykaliuta@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a couple of potential problem (static analysis) | expand |
On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote: >There is potential buffer overrun in kmod_builtin_iter_get_modname() >for the name of length PATH_MAX. The required buffer size is >PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write >right beyond the buffer. this doesn't look correct. "with len == PATH_MAX" we will actually return an error. What indeed is happening is truncation: since we are not reserving 1 char for NUL termination, we will truncate the name. If we update the commit message to state the right reasoning, then we can land this patch. I don't see any buffer overflow here, but I may be missing something. thanks LUcas De Marchi > >Check the length against PATH_MAX - 1. > >Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> >--- > libkmod/libkmod-builtin.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c >index a002cb5ee2c6..3d4d77ab29b3 100644 >--- a/libkmod/libkmod-builtin.c >+++ b/libkmod/libkmod-builtin.c >@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter, > > len = dot - line; > >- if (len >= PATH_MAX) { >+ if (len >= PATH_MAX - 1) { > sv_errno = ENAMETOOLONG; > goto fail; > } >-- >2.31.1 >
On Wed, Jun 09, 2021 at 10:10:53AM -0700, Lucas De Marchi wrote: >On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote: >>There is potential buffer overrun in kmod_builtin_iter_get_modname() >>for the name of length PATH_MAX. The required buffer size is >>PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write >>right beyond the buffer. > >this doesn't look correct. "with len == PATH_MAX" we will actually >return an error. > >What indeed is happening is truncation: since we are not reserving 1 >char for NUL termination, we will truncate the name. If we update the >commit message to state the right reasoning, then we can land this patch. > >I don't see any buffer overflow here, but I may be missing something. another thing... what is your git-sendemail setup? This is putting patch 2 as a reply to patch 1 and that breaks b4. See: https://lore.kernel.org/linux-modules/20210608062923.94017-1-ykaliuta@redhat.com/T/#u Lucas De Marchi > >thanks >LUcas De Marchi > >> >>Check the length against PATH_MAX - 1. >> >>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> >>--- >>libkmod/libkmod-builtin.c | 2 +- >>1 file changed, 1 insertion(+), 1 deletion(-) >> >>diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c >>index a002cb5ee2c6..3d4d77ab29b3 100644 >>--- a/libkmod/libkmod-builtin.c >>+++ b/libkmod/libkmod-builtin.c >>@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter, >> >> len = dot - line; >> >>- if (len >= PATH_MAX) { >>+ if (len >= PATH_MAX - 1) { >> sv_errno = ENAMETOOLONG; >> goto fail; >> } >>-- >>2.31.1 >>
Hi, Lucas! My filters missed your replies, sorry :( On Wed, Jun 9, 2021 at 8:11 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote: > >There is potential buffer overrun in kmod_builtin_iter_get_modname() > >for the name of length PATH_MAX. The required buffer size is > >PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write > >right beyond the buffer. > > this doesn't look correct. "with len == PATH_MAX" we will actually > return an error. > > What indeed is happening is truncation: since we are not reserving 1 > char for NUL termination, we will truncate the name. If we update the > commit message to state the right reasoning, then we can land this patch. > > I don't see any buffer overflow here, but I may be missing something. Yes, you are right. I see the actual overflow is fixed by 1cab02ecf6ee ("libkmod: fix an overflow with wrong modules.builtin.modinfo") I should have checked some reports more carefully. Please, ignore the patch. > thanks > LUcas De Marchi > > > > >Check the length against PATH_MAX - 1. > > > >Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> > >--- > > libkmod/libkmod-builtin.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > >diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c > >index a002cb5ee2c6..3d4d77ab29b3 100644 > >--- a/libkmod/libkmod-builtin.c > >+++ b/libkmod/libkmod-builtin.c > >@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter, > > > > len = dot - line; > > > >- if (len >= PATH_MAX) { > >+ if (len >= PATH_MAX - 1) { > > sv_errno = ENAMETOOLONG; > > goto fail; > > } > >-- > >2.31.1 > > > -- WBR, Yauheni
On Wed, Jun 9, 2021 at 8:19 PM Lucas De Marchi <lucas.demarchi@intel.com> wrote: > > On Wed, Jun 09, 2021 at 10:10:53AM -0700, Lucas De Marchi wrote: > >On Tue, Jun 08, 2021 at 09:29:23AM +0300, Yauheni Kaliuta wrote: > >>There is potential buffer overrun in kmod_builtin_iter_get_modname() > >>for the name of length PATH_MAX. The required buffer size is > >>PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write > >>right beyond the buffer. > > > >this doesn't look correct. "with len == PATH_MAX" we will actually > >return an error. > > > >What indeed is happening is truncation: since we are not reserving 1 > >char for NUL termination, we will truncate the name. If we update the > >commit message to state the right reasoning, then we can land this patch. > > > >I don't see any buffer overflow here, but I may be missing something. > > another thing... what is your git-sendemail setup? This is putting patch > 2 as a reply to patch 1 and that breaks b4. See: > https://lore.kernel.org/linux-modules/20210608062923.94017-1-ykaliuta@redhat.com/T/#u Thanks, yes. I have thread = true, but send it after the cover letter with git send-email --to linux-modules@vger.kernel.org --cc lucas.de.marchi@gmail.com --in-reply-to=20210608062859.93959-1-ykaliuta@redhat.com 0001-libkmod-module-check-new_from_name-return-value-in-g.patch 0002-libkmod-builtin-consider-final-NIL-in-name-length-ch.patch So, the right way is to send all together, or disable 'thread'. > > Lucas De Marchi > > > > >thanks > >LUcas De Marchi > > > >> > >>Check the length against PATH_MAX - 1. > >> > >>Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> > >>--- > >>libkmod/libkmod-builtin.c | 2 +- > >>1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c > >>index a002cb5ee2c6..3d4d77ab29b3 100644 > >>--- a/libkmod/libkmod-builtin.c > >>+++ b/libkmod/libkmod-builtin.c > >>@@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter, > >> > >> len = dot - line; > >> > >>- if (len >= PATH_MAX) { > >>+ if (len >= PATH_MAX - 1) { > >> sv_errno = ENAMETOOLONG; > >> goto fail; > >> } > >>-- > >>2.31.1 > >> >
diff --git a/libkmod/libkmod-builtin.c b/libkmod/libkmod-builtin.c index a002cb5ee2c6..3d4d77ab29b3 100644 --- a/libkmod/libkmod-builtin.c +++ b/libkmod/libkmod-builtin.c @@ -246,7 +246,7 @@ bool kmod_builtin_iter_get_modname(struct kmod_builtin_iter *iter, len = dot - line; - if (len >= PATH_MAX) { + if (len >= PATH_MAX - 1) { sv_errno = ENAMETOOLONG; goto fail; }
There is potential buffer overrun in kmod_builtin_iter_get_modname() for the name of length PATH_MAX. The required buffer size is PATH_MAX, so `modname[len] = '\0'` with len == PATH_MAX will write right beyond the buffer. Check the length against PATH_MAX - 1. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> --- libkmod/libkmod-builtin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)