* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).