* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char [not found] <20201109130447.2080491-1-colin.king@canonical.com> @ 2020-11-10 1:07 ` Qian Cai 2020-11-10 4:57 ` Paul Gortmaker 0 siblings, 1 reply; 6+ messages in thread From: Qian Cai @ 2020-11-10 1:07 UTC (permalink / raw) To: Colin King, Paul Gortmaker, Paul E . McKenney, Stephen Rothwell Cc: kernel-janitors, linux-kernel, Linux Next Mailing List On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently the allocation of cpulist is based on the length of buf but does > not include the addition end of string '\0' terminator. Static analysis is > reporting this as a potential out-of-bounds access on cpulist. Fix this by > allocating enough space for the additional '\0' terminator. > > Addresses-Coverity: ("Out-of-bounds access") > Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") Yeah, this bad commit also introduced KASAN errors everywhere and then will disable lockdep that makes our linux-next CI miserable. Confirmed that this patch will fix it. > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > lib/cpumask.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/cpumask.c b/lib/cpumask.c > index 34ecb3005941..cb8a3ef0e73e 100644 > --- a/lib/cpumask.c > +++ b/lib/cpumask.c > @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask > *dstp) > { > int r; > char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ > - size_t len = strlen(buf); > + size_t len = strlen(buf) + 1; > bool early = !slab_is_available(); > > if (!strcmp(buf, "all")) { ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char 2020-11-10 1:07 ` [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char Qian Cai @ 2020-11-10 4:57 ` Paul Gortmaker 2020-11-10 15:24 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Paul Gortmaker @ 2020-11-10 4:57 UTC (permalink / raw) To: Qian Cai, Colin King, Paul E . McKenney, Stephen Rothwell Cc: kernel-janitors, linux-kernel, Linux Next Mailing List On 2020-11-09 8:07 p.m., Qian Cai wrote: > On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> Currently the allocation of cpulist is based on the length of buf but does >> not include the addition end of string '\0' terminator. Static analysis is >> reporting this as a potential out-of-bounds access on cpulist. Fix this by >> allocating enough space for the additional '\0' terminator. >> >> Addresses-Coverity: ("Out-of-bounds access") >> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") > > Yeah, this bad commit also introduced KASAN errors everywhere and then will > disable lockdep that makes our linux-next CI miserable. Confirmed that this > patch will fix it. I appreciate the reports reminding me why I hate touching string handling. But let us not lose sight of why linux-next exists. We want to encourage code to appear there as a sounding board before it goes mainline, so we can fix things and not pollute mainline git history with those trivialities. If you've decided to internalize linux-next as part of your CI, then great, but do note that does not elevate linux-next to some pristine status for the world at large. That only means you have to watch more closely what is going on. If you want to declare linux-next unbreakable -- well that would scare away others to get the multi-arch or multi-config coverage that they may not be able to do themselves. We are not going to do that. I have (hopefully) fixed the "bad commit" in v2 -- as part of the implicit linux-next rule "you broke it, you better fix it ASAP". But "bad" and "miserable" can be things that might scare people off of making use of linux-next for what it is meant to be for. And I am not OK with that. Thanks, Paul. -- > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> lib/cpumask.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/cpumask.c b/lib/cpumask.c >> index 34ecb3005941..cb8a3ef0e73e 100644 >> --- a/lib/cpumask.c >> +++ b/lib/cpumask.c >> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask >> *dstp) >> { >> int r; >> char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ >> - size_t len = strlen(buf); >> + size_t len = strlen(buf) + 1; >> bool early = !slab_is_available(); >> >> if (!strcmp(buf, "all")) { > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char 2020-11-10 4:57 ` Paul Gortmaker @ 2020-11-10 15:24 ` Paul E. McKenney 2020-11-10 15:34 ` Colin Ian King 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2020-11-10 15:24 UTC (permalink / raw) To: Paul Gortmaker Cc: Qian Cai, Colin King, Stephen Rothwell, kernel-janitors, linux-kernel, Linux Next Mailing List On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote: > > > On 2020-11-09 8:07 p.m., Qian Cai wrote: > > On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: > > > From: Colin Ian King <colin.king@canonical.com> > > > > > > Currently the allocation of cpulist is based on the length of buf but does > > > not include the addition end of string '\0' terminator. Static analysis is > > > reporting this as a potential out-of-bounds access on cpulist. Fix this by > > > allocating enough space for the additional '\0' terminator. > > > > > > Addresses-Coverity: ("Out-of-bounds access") > > > Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") > > > > Yeah, this bad commit also introduced KASAN errors everywhere and then will > > disable lockdep that makes our linux-next CI miserable. Confirmed that this > > patch will fix it. > > I appreciate the reports reminding me why I hate touching string handling. > > But let us not lose sight of why linux-next exists. We want to > encourage code to appear there as a sounding board before it goes > mainline, so we can fix things and not pollute mainline git history > with those trivialities. > > If you've decided to internalize linux-next as part of your CI, then > great, but do note that does not elevate linux-next to some pristine > status for the world at large. That only means you have to watch more > closely what is going on. > > If you want to declare linux-next unbreakable -- well that would scare > away others to get the multi-arch or multi-config coverage that they may > not be able to do themselves. We are not going to do that. > > I have (hopefully) fixed the "bad commit" in v2 -- as part of the > implicit linux-next rule "you broke it, you better fix it ASAP". > > But "bad" and "miserable" can be things that might scare people off of > making use of linux-next for what it is meant to be for. And I am not > OK with that. They would need to use much stronger language to scare me off. That said, what on earth is the point of running tests if they do not from time to time find bugs? ;-) Thanx, Paul > Thanks, > Paul. > -- > > > > > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > > --- > > > lib/cpumask.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/cpumask.c b/lib/cpumask.c > > > index 34ecb3005941..cb8a3ef0e73e 100644 > > > --- a/lib/cpumask.c > > > +++ b/lib/cpumask.c > > > @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask > > > *dstp) > > > { > > > int r; > > > char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ > > > - size_t len = strlen(buf); > > > + size_t len = strlen(buf) + 1; > > > bool early = !slab_is_available(); > > > if (!strcmp(buf, "all")) { > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char 2020-11-10 15:24 ` Paul E. McKenney @ 2020-11-10 15:34 ` Colin Ian King 2020-11-10 18:38 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Colin Ian King @ 2020-11-10 15:34 UTC (permalink / raw) To: paulmck, Paul Gortmaker Cc: Qian Cai, Stephen Rothwell, kernel-janitors, linux-kernel, Linux Next Mailing List On 10/11/2020 15:24, Paul E. McKenney wrote: > On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote: >> >> >> On 2020-11-09 8:07 p.m., Qian Cai wrote: >>> On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: >>>> From: Colin Ian King <colin.king@canonical.com> >>>> >>>> Currently the allocation of cpulist is based on the length of buf but does >>>> not include the addition end of string '\0' terminator. Static analysis is >>>> reporting this as a potential out-of-bounds access on cpulist. Fix this by >>>> allocating enough space for the additional '\0' terminator. >>>> >>>> Addresses-Coverity: ("Out-of-bounds access") >>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") >>> >>> Yeah, this bad commit also introduced KASAN errors everywhere and then will >>> disable lockdep that makes our linux-next CI miserable. Confirmed that this >>> patch will fix it. >> >> I appreciate the reports reminding me why I hate touching string handling. >> >> But let us not lose sight of why linux-next exists. We want to >> encourage code to appear there as a sounding board before it goes >> mainline, so we can fix things and not pollute mainline git history >> with those trivialities. >> >> If you've decided to internalize linux-next as part of your CI, then >> great, but do note that does not elevate linux-next to some pristine >> status for the world at large. That only means you have to watch more >> closely what is going on. >> >> If you want to declare linux-next unbreakable -- well that would scare >> away others to get the multi-arch or multi-config coverage that they may >> not be able to do themselves. We are not going to do that. >> >> I have (hopefully) fixed the "bad commit" in v2 -- as part of the >> implicit linux-next rule "you broke it, you better fix it ASAP". >> >> But "bad" and "miserable" can be things that might scare people off of >> making use of linux-next for what it is meant to be for. And I am not >> OK with that. > > They would need to use much stronger language to scare me off. That said, > what on earth is the point of running tests if they do not from time to > time find bugs? ;-) > > Thanx, Paul For me, part of the QA process is statically analyzing linux-next to catch bugs before they land in linux. I think other testing is equally worth while as catching bugs early saves time and money. Colin > >> Thanks, >> Paul. >> -- >> >>> >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>> --- >>>> lib/cpumask.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/cpumask.c b/lib/cpumask.c >>>> index 34ecb3005941..cb8a3ef0e73e 100644 >>>> --- a/lib/cpumask.c >>>> +++ b/lib/cpumask.c >>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask >>>> *dstp) >>>> { >>>> int r; >>>> char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ >>>> - size_t len = strlen(buf); >>>> + size_t len = strlen(buf) + 1; >>>> bool early = !slab_is_available(); >>>> if (!strcmp(buf, "all")) { >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char 2020-11-10 15:34 ` Colin Ian King @ 2020-11-10 18:38 ` Paul E. McKenney 2020-11-10 19:07 ` Colin Ian King 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2020-11-10 18:38 UTC (permalink / raw) To: Colin Ian King Cc: Paul Gortmaker, Qian Cai, Stephen Rothwell, kernel-janitors, linux-kernel, Linux Next Mailing List On Tue, Nov 10, 2020 at 03:34:05PM +0000, Colin Ian King wrote: > On 10/11/2020 15:24, Paul E. McKenney wrote: > > On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote: > >> > >> > >> On 2020-11-09 8:07 p.m., Qian Cai wrote: > >>> On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: > >>>> From: Colin Ian King <colin.king@canonical.com> > >>>> > >>>> Currently the allocation of cpulist is based on the length of buf but does > >>>> not include the addition end of string '\0' terminator. Static analysis is > >>>> reporting this as a potential out-of-bounds access on cpulist. Fix this by > >>>> allocating enough space for the additional '\0' terminator. > >>>> > >>>> Addresses-Coverity: ("Out-of-bounds access") > >>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") > >>> > >>> Yeah, this bad commit also introduced KASAN errors everywhere and then will > >>> disable lockdep that makes our linux-next CI miserable. Confirmed that this > >>> patch will fix it. > >> > >> I appreciate the reports reminding me why I hate touching string handling. > >> > >> But let us not lose sight of why linux-next exists. We want to > >> encourage code to appear there as a sounding board before it goes > >> mainline, so we can fix things and not pollute mainline git history > >> with those trivialities. > >> > >> If you've decided to internalize linux-next as part of your CI, then > >> great, but do note that does not elevate linux-next to some pristine > >> status for the world at large. That only means you have to watch more > >> closely what is going on. > >> > >> If you want to declare linux-next unbreakable -- well that would scare > >> away others to get the multi-arch or multi-config coverage that they may > >> not be able to do themselves. We are not going to do that. > >> > >> I have (hopefully) fixed the "bad commit" in v2 -- as part of the > >> implicit linux-next rule "you broke it, you better fix it ASAP". > >> > >> But "bad" and "miserable" can be things that might scare people off of > >> making use of linux-next for what it is meant to be for. And I am not > >> OK with that. > > > > They would need to use much stronger language to scare me off. That said, > > what on earth is the point of running tests if they do not from time to > > time find bugs? ;-) > > For me, part of the QA process is statically analyzing linux-next to > catch bugs before they land in linux. I think other testing is equally > worth while as catching bugs early saves time and money. All kidding aside, the fact that this appeared in -next was due to a mistake on my part, namely failing to push the changes before starting the test. Please accept my apologies, and I will continue to do my best to avoid this sort of thing. Thanx, Paul > Colin > > > > >> Thanks, > >> Paul. > >> -- > >> > >>> > >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >>>> --- > >>>> lib/cpumask.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/lib/cpumask.c b/lib/cpumask.c > >>>> index 34ecb3005941..cb8a3ef0e73e 100644 > >>>> --- a/lib/cpumask.c > >>>> +++ b/lib/cpumask.c > >>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask > >>>> *dstp) > >>>> { > >>>> int r; > >>>> char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ > >>>> - size_t len = strlen(buf); > >>>> + size_t len = strlen(buf) + 1; > >>>> bool early = !slab_is_available(); > >>>> if (!strcmp(buf, "all")) { > >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char 2020-11-10 18:38 ` Paul E. McKenney @ 2020-11-10 19:07 ` Colin Ian King 0 siblings, 0 replies; 6+ messages in thread From: Colin Ian King @ 2020-11-10 19:07 UTC (permalink / raw) To: paulmck Cc: Paul Gortmaker, Qian Cai, Stephen Rothwell, kernel-janitors, linux-kernel, Linux Next Mailing List On 10/11/2020 18:38, Paul E. McKenney wrote: > On Tue, Nov 10, 2020 at 03:34:05PM +0000, Colin Ian King wrote: >> On 10/11/2020 15:24, Paul E. McKenney wrote: >>> On Mon, Nov 09, 2020 at 11:57:15PM -0500, Paul Gortmaker wrote: >>>> >>>> >>>> On 2020-11-09 8:07 p.m., Qian Cai wrote: >>>>> On Mon, 2020-11-09 at 13:04 +0000, Colin King wrote: >>>>>> From: Colin Ian King <colin.king@canonical.com> >>>>>> >>>>>> Currently the allocation of cpulist is based on the length of buf but does >>>>>> not include the addition end of string '\0' terminator. Static analysis is >>>>>> reporting this as a potential out-of-bounds access on cpulist. Fix this by >>>>>> allocating enough space for the additional '\0' terminator. >>>>>> >>>>>> Addresses-Coverity: ("Out-of-bounds access") >>>>>> Fixes: 65987e67f7ff ("cpumask: add "last" alias for cpu list specifications") >>>>> >>>>> Yeah, this bad commit also introduced KASAN errors everywhere and then will >>>>> disable lockdep that makes our linux-next CI miserable. Confirmed that this >>>>> patch will fix it. >>>> >>>> I appreciate the reports reminding me why I hate touching string handling. >>>> >>>> But let us not lose sight of why linux-next exists. We want to >>>> encourage code to appear there as a sounding board before it goes >>>> mainline, so we can fix things and not pollute mainline git history >>>> with those trivialities. >>>> >>>> If you've decided to internalize linux-next as part of your CI, then >>>> great, but do note that does not elevate linux-next to some pristine >>>> status for the world at large. That only means you have to watch more >>>> closely what is going on. >>>> >>>> If you want to declare linux-next unbreakable -- well that would scare >>>> away others to get the multi-arch or multi-config coverage that they may >>>> not be able to do themselves. We are not going to do that. >>>> >>>> I have (hopefully) fixed the "bad commit" in v2 -- as part of the >>>> implicit linux-next rule "you broke it, you better fix it ASAP". >>>> >>>> But "bad" and "miserable" can be things that might scare people off of >>>> making use of linux-next for what it is meant to be for. And I am not >>>> OK with that. >>> >>> They would need to use much stronger language to scare me off. That said, >>> what on earth is the point of running tests if they do not from time to >>> time find bugs? ;-) >> >> For me, part of the QA process is statically analyzing linux-next to >> catch bugs before they land in linux. I think other testing is equally >> worth while as catching bugs early saves time and money. > > All kidding aside, the fact that this appeared in -next was due to a > mistake on my part, namely failing to push the changes before starting > the test. Please accept my apologies, and I will continue to do my > best to avoid this sort of thing. > > Thanx, Paul No problem. I'm glad we have tools to catch issues like this. Colin > >> Colin >> >>> >>>> Thanks, >>>> Paul. >>>> -- >>>> >>>>> >>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>>>> --- >>>>>> lib/cpumask.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/cpumask.c b/lib/cpumask.c >>>>>> index 34ecb3005941..cb8a3ef0e73e 100644 >>>>>> --- a/lib/cpumask.c >>>>>> +++ b/lib/cpumask.c >>>>>> @@ -185,7 +185,7 @@ int __ref cpulist_parse(const char *buf, struct cpumask >>>>>> *dstp) >>>>>> { >>>>>> int r; >>>>>> char *cpulist, last_cpu[5]; /* NR_CPUS <= 9999 */ >>>>>> - size_t len = strlen(buf); >>>>>> + size_t len = strlen(buf) + 1; >>>>>> bool early = !slab_is_available(); >>>>>> if (!strcmp(buf, "all")) { >>>>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-10 19:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20201109130447.2080491-1-colin.king@canonical.com>
2020-11-10 1:07 ` [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char Qian Cai
2020-11-10 4:57 ` Paul Gortmaker
2020-11-10 15:24 ` Paul E. McKenney
2020-11-10 15:34 ` Colin Ian King
2020-11-10 18:38 ` Paul E. McKenney
2020-11-10 19:07 ` Colin Ian King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox