| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5592555C.4080702@intel.com>
Date: Tue, 30 Jun 2015 16:37:48 +0800
From: Pan Xinhui <xinhuix.pan@...el.com>
To: Yury Norov <yury.norov@...il.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Rasmus Villemoes <linux@...musvillemoes.dk>, tj@...nel.org,
peterz@...radead.org, sudeep.holla@....com, mina86@...a86.com,
"mnipxh@....com" <mnipxh@....com>,
Alexey Klimov <klimov.linux@...il.com>
Subject: Re: [PATCH] lib/bitmap.c: return -EINVAL for grouping errors in __bitmap_parselist
hi, Yury
On 2015年06月30日 16:32, Yury Norov wrote:
> 2015-07-01 4:37 GMT+03:00 Pan Xinhui <xinhuix.pan@...el.com>:
>> hi, Yury
>> thanks for your nice reply.
>>
>> On 2015年06月29日 21:39, Yury Norov wrote:
>>>>
>>>> Sometimes the input from user may cause an unexpected result.
>>>
>>>
>>> Could you please provide specific example?
>>>
>> I wrote some scripts to do some tests about irqs.
>> echo "1-3," > /proc/irq/<xxx>/smp_affinity_list
>> this command ends with ',' by mistake.
>> actually __bitmap_parselist() will report "0-3" for the final result which
>> is wrong.
>>
>
> Hmm...
> I don't think this is wrong passing echo "1-3,".
> With or without a comma, the final result must be the same.
> More flexible format is useful for hard scripts (for your one).
> It's not too difficult to imagine a script producing a line:
> "1-24, , ,,, , 12-64, 92,92,92,,,"
> And I don't think we should reject user with this once the range is valid.
> Even more, to spend a time writing some additional code for it, and make
> user spend his time as well.
>
> I just tried
> cd /home/yury///./././/work
> and it works perfectly well for me, and it's fine.
>
> The true problem is that a and b variables
> goes zero after comma, and EOL after comma just takes it:
> 514 do {
> ...
> 517 a = b = 0; //
> <--- comma makes it 0 here
> ...
> 520 while (buflen) {
> ...
> 539 /* A '\0' or a ',' signal the end of a cpu# or range */
> 540 if (c == '\0' || c == ',') //
> <---here we just break after '\0'
> 541 break;
> 559 }
> ...
> 565 while (a <= b) {
> 566 set_bit(a, maskp); // <--- and
> here we set unneeded 0 bit.
> 567 a++;
> 568 }
>
> So currently, "1-3,\0" is the same as "1-3,0,\0". And this is definitely wrong.
>
yes, you are right.
current codes did not check if there is any digit between ',' or '\0'.
I has sent out patch V2, which rewrite two functions.
could you help have a code review if you have free time? thanks for your nice reply :)
thanks,
xinhui
>>
>>>>
>>>> just like __bitmap_parse, we return -EINVAL if there is no avaiable digit
>>>> in each
>>>> parsing procedures.
>>>>
>>>> Signed-off-by: Pan Xinhui <xinhuix.pan@...el.com>
>>>
>>>
>>> Hello, Pan.
>>>
>>> (Adding Alexey Klimov, Rasmus Villemoes)
>>>
>>>> ---
>>>> lib/bitmap.c | 7 +++++--
>>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/bitmap.c b/lib/bitmap.c
>>>> index 64c0926..995fca2 100644
>>>> --- a/lib/bitmap.c
>>>> +++ b/lib/bitmap.c
>>>> @@ -504,7 +504,7 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>> int nmaskbits)
>>>> {
>>>> unsigned a, b;
>>>> - int c, old_c, totaldigits;
>>>> + int c, old_c, totaldigits, ndigits;
>>>> const char __user __force *ubuf = (const char __user __force
>>>> *)buf;
>>>> int exp_digit, in_range;
>>>>
>>>> @@ -514,6 +514,7 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>> exp_digit = 1;
>>>> in_range = 0;
>>>> a = b = 0;
>>>> + ndigits = 0;
>>>>
>>>> /* Get the next cpu# or a range of cpu#'s */
>>>> while (buflen) {
>>>> @@ -555,8 +556,10 @@ static int __bitmap_parselist(const char *buf,
>>>> unsigned int buflen,
>>>> if (!in_range)
>>>> a = b;
>>>> exp_digit = 0;
>>>> - totaldigits++;
>>>> + ndigits++; totaldigits++;
>>>
>>>
>>> I'm not happy with joining two statements to a single line.
>>> Maybe sometimes it's OK for loop iterators like
>>>
>>> while (a[i][j]) {
>>> i++; j++;
>>> }
>>>
>>> But here it looks nasty. Anyway, it's minor.
>>>
>>
>> thanks for pointing out my mistake about the code style :)
>>
>>>> }
>>>> + if (ndigits == 0)
>>>> + return -EINVAL;
>>>
>>>
>>> You can avoid in-loop incrementation of ndigits if you'll
>>> save current totaldigits to ndigits before loop, and check
>>> ndigits against totaldigits after the loop:
>>>
>>> ndigits = totaldigits;
>>> while (...) {
>>> ...
>>> totaldigits++;
>>> }
>>>
>>> if (ndigits == totaldigits)
>>> return -EINVAL;
>>>
>>> Maybe it's a good point to rework initial __bitmap_parse() similar way...
>>>
>>
>> your advice is a good idea, thanks.
>> I am also thinking if we can rewrite them into one function for common
>> codes.
>>
>> thanks for your reply again :)
>>
>> thanks
>> xinhui
>>
>>
>>>> if (!(a <= b))
>>>> return -EINVAL;
>>>> if (b >= nmaskbits)
>>>> --
>>>> 1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists