* [PATCH 0/2] Fix a couple of potential problem (static analysis) @ 2021-06-08 6:28 Yauheni Kaliuta 2021-06-08 6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta 0 siblings, 1 reply; 8+ messages in thread From: Yauheni Kaliuta @ 2021-06-08 6:28 UTC (permalink / raw) To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta Yauheni Kaliuta (2): libkmod-module: check "new_from_name" return value in get_builtin libkmod-builtin: consider final NIL in name length check libkmod/libkmod-builtin.c | 2 +- libkmod/libkmod-module.c | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin 2021-06-08 6:28 [PATCH 0/2] Fix a couple of potential problem (static analysis) Yauheni Kaliuta @ 2021-06-08 6:29 ` Yauheni Kaliuta 2021-06-08 6:29 ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta 2021-06-09 17:22 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi 0 siblings, 2 replies; 8+ messages in thread From: Yauheni Kaliuta @ 2021-06-08 6:29 UTC (permalink / raw) To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta kmod_module_new_from_name() may fail and return error value. It is handled properly across the code, but in this particular place the check is missing. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> --- libkmod/libkmod-module.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c index b6320cc87e80..6e0ff1a99604 100644 --- a/libkmod/libkmod-module.c +++ b/libkmod/libkmod-module.c @@ -2912,7 +2912,10 @@ int kmod_module_get_builtin(struct kmod_ctx *ctx, struct kmod_list **list) goto fail; } - kmod_module_new_from_name(ctx, modname, &mod); + err = kmod_module_new_from_name(ctx, modname, &mod); + if (err < 0) + goto fail; + kmod_module_set_builtin(mod, true); *list = kmod_list_append(*list, mod); -- 2.31.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] libkmod-builtin: consider final NIL in name length check 2021-06-08 6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta @ 2021-06-08 6:29 ` Yauheni Kaliuta 2021-06-09 17:10 ` Lucas De Marchi 2021-06-09 17:22 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi 1 sibling, 1 reply; 8+ messages in thread From: Yauheni Kaliuta @ 2021-06-08 6:29 UTC (permalink / raw) To: linux-modules; +Cc: lucas.de.marchi, Yauheni Kaliuta 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(-) 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check 2021-06-08 6:29 ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta @ 2021-06-09 17:10 ` Lucas De Marchi 2021-06-09 17:18 ` Lucas De Marchi 2021-06-22 6:22 ` Yauheni Kaliuta 0 siblings, 2 replies; 8+ messages in thread From: Lucas De Marchi @ 2021-06-09 17:10 UTC (permalink / raw) To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check 2021-06-09 17:10 ` Lucas De Marchi @ 2021-06-09 17:18 ` Lucas De Marchi 2021-06-22 6:31 ` Yauheni Kaliuta 2021-06-22 6:22 ` Yauheni Kaliuta 1 sibling, 1 reply; 8+ messages in thread From: Lucas De Marchi @ 2021-06-09 17:18 UTC (permalink / raw) To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi 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 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check 2021-06-09 17:18 ` Lucas De Marchi @ 2021-06-22 6:31 ` Yauheni Kaliuta 0 siblings, 0 replies; 8+ messages in thread From: Yauheni Kaliuta @ 2021-06-22 6:31 UTC (permalink / raw) To: Lucas De Marchi; +Cc: linux-modules, lucas.de.marchi 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 > >> > -- WBR, Yauheni ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] libkmod-builtin: consider final NIL in name length check 2021-06-09 17:10 ` Lucas De Marchi 2021-06-09 17:18 ` Lucas De Marchi @ 2021-06-22 6:22 ` Yauheni Kaliuta 1 sibling, 0 replies; 8+ messages in thread From: Yauheni Kaliuta @ 2021-06-22 6:22 UTC (permalink / raw) To: Lucas De Marchi; +Cc: linux-modules, lucas.de.marchi 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin 2021-06-08 6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta 2021-06-08 6:29 ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta @ 2021-06-09 17:22 ` Lucas De Marchi 1 sibling, 0 replies; 8+ messages in thread From: Lucas De Marchi @ 2021-06-09 17:22 UTC (permalink / raw) To: Yauheni Kaliuta; +Cc: linux-modules, lucas.de.marchi On Tue, Jun 08, 2021 at 09:29:22AM +0300, Yauheni Kaliuta wrote: >kmod_module_new_from_name() may fail and return error value. It is >handled properly across the code, but in this particular place the >check is missing. > >Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> applied, thanks Lucas De Marchi >--- > libkmod/libkmod-module.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > >diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c >index b6320cc87e80..6e0ff1a99604 100644 >--- a/libkmod/libkmod-module.c >+++ b/libkmod/libkmod-module.c >@@ -2912,7 +2912,10 @@ int kmod_module_get_builtin(struct kmod_ctx *ctx, struct kmod_list **list) > goto fail; > } > >- kmod_module_new_from_name(ctx, modname, &mod); >+ err = kmod_module_new_from_name(ctx, modname, &mod); >+ if (err < 0) >+ goto fail; >+ > kmod_module_set_builtin(mod, true); > > *list = kmod_list_append(*list, mod); >-- >2.31.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-06-22 6:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-08 6:28 [PATCH 0/2] Fix a couple of potential problem (static analysis) Yauheni Kaliuta 2021-06-08 6:29 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Yauheni Kaliuta 2021-06-08 6:29 ` [PATCH 2/2] libkmod-builtin: consider final NIL in name length check Yauheni Kaliuta 2021-06-09 17:10 ` Lucas De Marchi 2021-06-09 17:18 ` Lucas De Marchi 2021-06-22 6:31 ` Yauheni Kaliuta 2021-06-22 6:22 ` Yauheni Kaliuta 2021-06-09 17:22 ` [PATCH 1/2] libkmod-module: check "new_from_name" return value in get_builtin Lucas De Marchi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.