[PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Colin King]

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")
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")) {
-- 
2.28.0

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Qian Cai]

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")) {

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Paul Gortmaker]

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")) {
> 

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Paul E. McKenney]

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")) {
> > 

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Colin Ian King]

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")) {
>>>

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Paul E. McKenney]

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")) {
> >>>
> 

Re: [PATCH][next] cpumask: allocate enough space for string and trailing '\0' char

[Colin Ian King]

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")) {
>>>>>
>>