Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 6, 2018, at 6:22 PM, Eric W. Biederman  wrote:
> 
> Ilya Matveychikov  writes:
> 
>>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman  
>>> wrote:
>>> 
>>> Ilya Matveychikov  writes:
>>> 
>>>> Just CC’ed to some of maintainers.
>>>> 
>>>> $ perl scripts/get_maintainer.pl 
>>>> fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>>>> Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
>>>> infrastructure))
>>>> linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and 
>>>> infrastructure))
>>>> linux-kernel@vger.kernel.org (open list)
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>>>> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> *Snort*
>>> 
>>> You clearly have not read may_mount.  Your modified code still
>>> let's unprivileged users in.  So even if all of Al's good objections
>>> were not applicable this change would still be buggy and wrong.
>>> 
>>> Nacked-by: "Eric W. Biederman" 
>> 
>> 
>> Don’t get me wrong but may_mount() is:
>> 
>> static inline bool may_mount(void)
>> {
>>return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
>> }
>> 
>> What do you mean by "You clearly have not read may_mount”? The only thing 
>> that
>> can affect may_mount result (as mentioned earlier) is that task’s NS 
>> capability
>> might be changed by security_sb_mount() hook.
>> 
>> So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering 
>> to
>> ksys_mount() but getting it with the security_sb_mount() hook?
> 
> I mean it works for unprivileged users.
> 
> You can try "unshare -Urm" on a reasonably recent kernel and it will
> work and you can then mount and unmount things.
> 
> Strictly speaking it only works if you have the appropriate set of
> capabilities in your user namespace.  But that does not imply you are a
> privileged user in the broader sense.
> 
> Any user can create a user namespace, and become the root user
> in a user namespace.  The root user in a user namespace can create
> a mount namespace.  The root user in a user namespace can mount
> and unmount filesystems in their namespace.
> 
> Or in net anyone can call mount and get past the may_mount test.
> 
> Without reducing who can cause the kernel allocation moving the test is
> pointless.
> 

Ok, now I see. No reason to make change as it doesn’t really prevents users
of doing mount() using they own namespaces.

Thank you for the explanation.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 6, 2018, at 6:22 PM, Eric W. Biederman  wrote:
> 
> Ilya Matveychikov  writes:
> 
>>> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman  
>>> wrote:
>>> 
>>> Ilya Matveychikov  writes:
>>> 
>>>> Just CC’ed to some of maintainers.
>>>> 
>>>> $ perl scripts/get_maintainer.pl 
>>>> fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>>>> Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
>>>> infrastructure))
>>>> linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and 
>>>> infrastructure))
>>>> linux-kernel@vger.kernel.org (open list)
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>>>> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> *Snort*
>>> 
>>> You clearly have not read may_mount.  Your modified code still
>>> let's unprivileged users in.  So even if all of Al's good objections
>>> were not applicable this change would still be buggy and wrong.
>>> 
>>> Nacked-by: "Eric W. Biederman" 
>> 
>> 
>> Don’t get me wrong but may_mount() is:
>> 
>> static inline bool may_mount(void)
>> {
>>return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
>> }
>> 
>> What do you mean by "You clearly have not read may_mount”? The only thing 
>> that
>> can affect may_mount result (as mentioned earlier) is that task’s NS 
>> capability
>> might be changed by security_sb_mount() hook.
>> 
>> So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering 
>> to
>> ksys_mount() but getting it with the security_sb_mount() hook?
> 
> I mean it works for unprivileged users.
> 
> You can try "unshare -Urm" on a reasonably recent kernel and it will
> work and you can then mount and unmount things.
> 
> Strictly speaking it only works if you have the appropriate set of
> capabilities in your user namespace.  But that does not imply you are a
> privileged user in the broader sense.
> 
> Any user can create a user namespace, and become the root user
> in a user namespace.  The root user in a user namespace can create
> a mount namespace.  The root user in a user namespace can mount
> and unmount filesystems in their namespace.
> 
> Or in net anyone can call mount and get past the may_mount test.
> 
> Without reducing who can cause the kernel allocation moving the test is
> pointless.
> 

Ok, now I see. No reason to make change as it doesn’t really prevents users
of doing mount() using they own namespaces.

Thank you for the explanation.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman  wrote:
> 
> Ilya Matveychikov  writes:
> 
>> Just CC’ed to some of maintainers.
>> 
>> $ perl scripts/get_maintainer.pl 
>> fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>> Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
>> infrastructure))
>> linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and 
>> infrastructure))
>> linux-kernel@vger.kernel.org (open list)
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> *Snort*
> 
> You clearly have not read may_mount.  Your modified code still
> let's unprivileged users in.  So even if all of Al's good objections
> were not applicable this change would still be buggy and wrong.
> 
> Nacked-by: "Eric W. Biederman" 


Don’t get me wrong but may_mount() is:

static inline bool may_mount(void)
{
return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
}

What do you mean by "You clearly have not read may_mount”? The only thing that
can affect may_mount result (as mentioned earlier) is that task’s NS capability
might be changed by security_sb_mount() hook.

So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering to
ksys_mount() but getting it with the security_sb_mount() hook?

This is the only case I see that using may_mount() before security_sb_mount()
is wrong. This was the point?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 11:56 PM, Eric W. Biederman  wrote:
> 
> Ilya Matveychikov  writes:
> 
>> Just CC’ed to some of maintainers.
>> 
>> $ perl scripts/get_maintainer.pl 
>> fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
>> Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
>> infrastructure))
>> linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and 
>> infrastructure))
>> linux-kernel@vger.kernel.org (open list)
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> *Snort*
> 
> You clearly have not read may_mount.  Your modified code still
> let's unprivileged users in.  So even if all of Al's good objections
> were not applicable this change would still be buggy and wrong.
> 
> Nacked-by: "Eric W. Biederman" 


Don’t get me wrong but may_mount() is:

static inline bool may_mount(void)
{
return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
}

What do you mean by "You clearly have not read may_mount”? The only thing that
can affect may_mount result (as mentioned earlier) is that task’s NS capability
might be changed by security_sb_mount() hook.

So, do you think that is’s possible to NOT have CAP_SYS_ADMIN while entering to
ksys_mount() but getting it with the security_sb_mount() hook?

This is the only case I see that using may_mount() before security_sb_mount()
is wrong. This was the point?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 4:28 PM, David Sterba  wrote:
> 
>> BTW, sys_umount() has this check in the right place - before doing anything.
>> So, why not to have the same logic for mount/umount?
> 
> What if the check is not equivalent to the one done later? may_mount
> needs namespace, it will be available at umount time but not necessarily
> during mount due to the security hooks.

Might be the issue, you’re right. I can’t tell it for sure as I’m not so
familiar with linux/fs code.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 4:28 PM, David Sterba  wrote:
> 
>> BTW, sys_umount() has this check in the right place - before doing anything.
>> So, why not to have the same logic for mount/umount?
> 
> What if the check is not equivalent to the one done later? may_mount
> needs namespace, it will be available at umount time but not necessarily
> during mount due to the security hooks.

Might be the issue, you’re right. I can’t tell it for sure as I’m not so
familiar with linux/fs code.

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:53 PM, Al Viro  wrote:
> 
> On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
>> 
>>> On Jun 5, 2018, at 3:26 PM, Al Viro  wrote:
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>>>> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> I'm sorry, but there are arseloads of unpriveleged syscalls that do the 
>>> same,
>>> starting with read() from procfs files.  So what the hell does it buy?
>> 
>> Means that if all do the same shit no reason to fix it? Sounds weird...
> 
> Fix *what*?  You do realize that there's no permission checks to stop e.g.
> stat(2) from copying the pathname in, right?  With user-supplied contents,
> even...
> 
> If you depend upon preventing kmalloc'ed temporary allocations filled
> with user-supplied data, you are screwed, plain and simple.  It really can't
> be prevented, in a lot of ways that are much less exotic than mount(2).
> Most of syscall arguments are copied in, before we get any permission
> checks.  It does happen and it will happen - examining them while they are
> still in userland is a nightmare in a lot of respects, starting with
> security.

I agree that it’s impossible to completely avoid this kind of allocations
and examining data in user-land will be the bigger problem than copying
arguments to the kernel. But aside of that what’s wrong with the idea of
having the permission check before doing any kind of work?

BTW, sys_umount() has this check in the right place - before doing anything.
So, why not to have the same logic for mount/umount?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:53 PM, Al Viro  wrote:
> 
> On Tue, Jun 05, 2018 at 03:35:55PM +0400, Ilya Matveychikov wrote:
>> 
>>> On Jun 5, 2018, at 3:26 PM, Al Viro  wrote:
>>>> 
>>>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>>>> wrote:
>>>>> 
>>>>> Early check for mount permissions prevents possible allocation of 3
>>>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>>>> spraying the kernel heap.
>>> 
>>> I'm sorry, but there are arseloads of unpriveleged syscalls that do the 
>>> same,
>>> starting with read() from procfs files.  So what the hell does it buy?
>> 
>> Means that if all do the same shit no reason to fix it? Sounds weird...
> 
> Fix *what*?  You do realize that there's no permission checks to stop e.g.
> stat(2) from copying the pathname in, right?  With user-supplied contents,
> even...
> 
> If you depend upon preventing kmalloc'ed temporary allocations filled
> with user-supplied data, you are screwed, plain and simple.  It really can't
> be prevented, in a lot of ways that are much less exotic than mount(2).
> Most of syscall arguments are copied in, before we get any permission
> checks.  It does happen and it will happen - examining them while they are
> still in userland is a nightmare in a lot of respects, starting with
> security.

I agree that it’s impossible to completely avoid this kind of allocations
and examining data in user-land will be the bigger problem than copying
arguments to the kernel. But aside of that what’s wrong with the idea of
having the permission check before doing any kind of work?

BTW, sys_umount() has this check in the right place - before doing anything.
So, why not to have the same logic for mount/umount?

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:26 PM, Al Viro  wrote:
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
> starting with read() from procfs files.  So what the hell does it buy?

Means that if all do the same shit no reason to fix it? Sounds weird...

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

> On Jun 5, 2018, at 3:26 PM, Al Viro  wrote:
>> 
>>> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  
>>> wrote:
>>> 
>>> Early check for mount permissions prevents possible allocation of 3
>>> pages from kmalloc() pool by unpriveledged user which can be used for
>>> spraying the kernel heap.
> 
> I'm sorry, but there are arseloads of unpriveleged syscalls that do the same,
> starting with read() from procfs files.  So what the hell does it buy?

Means that if all do the same shit no reason to fix it? Sounds weird...

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

Just CC’ed to some of maintainers.

$ perl scripts/get_maintainer.pl 
fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
infrastructure))
linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
linux-kernel@vger.kernel.org (open list)

> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  wrote:
> 
> Early check for mount permissions prevents possible allocation of 3
> pages from kmalloc() pool by unpriveledged user which can be used for
> spraying the kernel heap.
> 
> Signed-off-by: Ilya V. Matveychikov 
> ---
> fs/namespace.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5f75969adff1..1ef8feb2de2a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user 
> *dir_name, char __user *type,
>   char *kernel_dev;
>   void *options;
> 
> + if (!may_mount())
> + return -EPERM;
> +
>   kernel_type = copy_mount_string(type);
>   ret = PTR_ERR(kernel_type);
>   if (IS_ERR(kernel_type))
> --
> 2.17.0
> 

Re: [PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

Just CC’ed to some of maintainers.

$ perl scripts/get_maintainer.pl 
fs/0001-ksys_mount-check-for-permissions-before-resource-all.patch
Alexander Viro  (maintainer:FILESYSTEMS (VFS and 
infrastructure))
linux-fsde...@vger.kernel.org (open list:FILESYSTEMS (VFS and infrastructure))
linux-kernel@vger.kernel.org (open list)

> On Jun 5, 2018, at 6:00 AM, Ilya Matveychikov  wrote:
> 
> Early check for mount permissions prevents possible allocation of 3
> pages from kmalloc() pool by unpriveledged user which can be used for
> spraying the kernel heap.
> 
> Signed-off-by: Ilya V. Matveychikov 
> ---
> fs/namespace.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5f75969adff1..1ef8feb2de2a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user 
> *dir_name, char __user *type,
>   char *kernel_dev;
>   void *options;
> 
> + if (!may_mount())
> + return -EPERM;
> +
>   kernel_type = copy_mount_string(type);
>   ret = PTR_ERR(kernel_type);
>   if (IS_ERR(kernel_type))
> --
> 2.17.0
> 

[PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

Early check for mount permissions prevents possible allocation of 3
pages from kmalloc() pool by unpriveledged user which can be used for
spraying the kernel heap.

Signed-off-by: Ilya V. Matveychikov 
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5f75969adff1..1ef8feb2de2a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user 
*dir_name, char __user *type,
char *kernel_dev;
void *options;

+   if (!may_mount())
+   return -EPERM;
+
kernel_type = copy_mount_string(type);
ret = PTR_ERR(kernel_type);
if (IS_ERR(kernel_type))
--
2.17.0

[PATCH] ksys_mount: check for permissions before resource allocation

[Ilya Matveychikov]

Early check for mount permissions prevents possible allocation of 3
pages from kmalloc() pool by unpriveledged user which can be used for
spraying the kernel heap.

Signed-off-by: Ilya V. Matveychikov 
---
 fs/namespace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/namespace.c b/fs/namespace.c
index 5f75969adff1..1ef8feb2de2a 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3046,6 +3046,9 @@ int ksys_mount(char __user *dev_name, char __user 
*dir_name, char __user *type,
char *kernel_dev;
void *options;

+   if (!may_mount())
+   return -EPERM;
+
kernel_type = copy_mount_string(type);
ret = PTR_ERR(kernel_type);
if (IS_ERR(kernel_type))
--
2.17.0

Re: [RFC PATCH 0/3] Introduce kernel small arrays (KSA)

[Ilya Matveychikov]

> On Oct 4, 2017, at 7:22 PM, Ben Hutchings <ben.hutchi...@codethink.co.uk> 
> wrote:
> 
> On Tue, 2017-09-19 at 12:30 +0400, Ilya Matveychikov wrote:
>> Hi guys,
>> 
>> Please review the approach of using small fixed-sized arrays to improve
>> parsing of values like get_options() does.
>> 
>> This comes to me after fixing an overflow in get_options(). See the thread
>> for details: https://lkml.org/lkml/2017/5/22/581
>> 
>> If the approach is OK I’ll suggest to replace all of get_options() calls
>> to ksa_parse_ints() and remove get_options() at all.
> 
> You didn't cc the patches to me, and I can't find patch 3/3 at all.

Thanks for you answer. I posted them on list, except the last one as what I
wanted is to get the feedback first. I CC’d you as you found a problem with
my patch for get_options() before (read out of bounds).

> 
>  don't think the KSA() macro should be casting its argument.  Where the
> cast is necessary, it ought to be explicit in the caller.

KSA(x) it’s just a simple way to cast from custom-defined small array
to generic one. Do you think that it’s better to use explicit casting:

ksa_parse_foo(..., (struct ksmall_array *)_custom_ksa)

Instead of:

ksa_parse_foo(..., KSA(_custom_ksa))

Not sure...

> 
> Similarly I think the BUILD_BUG_ON() in ksa_build_check() doesn't belong
> there, but in whichever caller of ksa_parse_ints() requires struct
> ksmall_array to have the same layout as a simple array of unsigned int.

Not sure that I understand your point. The purpose of ksa_build_check() as
I wrote it is to to do compile-time check for sizeof(struct ksmall_array)
to fit sizeof(unsigned int). Note that ksmall_array{} is the header for any
possible “small” array build by using the KSA_DECLARE() macro.

Re: [RFC PATCH 0/3] Introduce kernel small arrays (KSA)

[Ilya Matveychikov]

> On Oct 4, 2017, at 7:22 PM, Ben Hutchings  
> wrote:
> 
> On Tue, 2017-09-19 at 12:30 +0400, Ilya Matveychikov wrote:
>> Hi guys,
>> 
>> Please review the approach of using small fixed-sized arrays to improve
>> parsing of values like get_options() does.
>> 
>> This comes to me after fixing an overflow in get_options(). See the thread
>> for details: https://lkml.org/lkml/2017/5/22/581
>> 
>> If the approach is OK I’ll suggest to replace all of get_options() calls
>> to ksa_parse_ints() and remove get_options() at all.
> 
> You didn't cc the patches to me, and I can't find patch 3/3 at all.

Thanks for you answer. I posted them on list, except the last one as what I
wanted is to get the feedback first. I CC’d you as you found a problem with
my patch for get_options() before (read out of bounds).

> 
>  don't think the KSA() macro should be casting its argument.  Where the
> cast is necessary, it ought to be explicit in the caller.

KSA(x) it’s just a simple way to cast from custom-defined small array
to generic one. Do you think that it’s better to use explicit casting:

ksa_parse_foo(..., (struct ksmall_array *)_custom_ksa)

Instead of:

ksa_parse_foo(..., KSA(_custom_ksa))

Not sure...

> 
> Similarly I think the BUILD_BUG_ON() in ksa_build_check() doesn't belong
> there, but in whichever caller of ksa_parse_ints() requires struct
> ksmall_array to have the same layout as a simple array of unsigned int.

Not sure that I understand your point. The purpose of ksa_build_check() as
I wrote it is to to do compile-time check for sizeof(struct ksmall_array)
to fit sizeof(unsigned int). Note that ksmall_array{} is the header for any
possible “small” array build by using the KSA_DECLARE() macro.

Re: [PATCH 4.4 03/26] lib/cmdline.c: fix get_options() overflow while parsing ranges

[Ilya Matveychikov]

> On Jun 29, 2017, at 7:24 PM, Ben Hutchings <ben.hutchi...@codethink.co.uk> 
> wrote:
> 
> On Tue, 2017-06-27 at 14:49 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>> 
>> --
>> 
>> From: Ilya Matveychikov <matvejchi...@gmail.com>
>> 
>> commit a91e0f680bcd9e10c253ae8b62462a38bd48f09f upstream.
>> 
>> When using get_options() it's possible to specify a range of numbers,
>> like 1-100500.  The problem is that it doesn't track array size while
>> calling internally to get_range() which iterates over the range and
>> fills the memory with numbers.
> [...]
>> --- a/lib/cmdline.c
>> +++ b/lib/cmdline.c
>> @@ -22,14 +22,14 @@
>>  *   the values[M, M+1, ..., N] into the ints array in get_options.
>>  */
>> 
>> -static int get_range(char **str, int *pint)
>> +static int get_range(char **str, int *pint, int n)
>> {
>>  int x, inc_counter, upper_range;
>> 
>>  (*str)++;
>>  upper_range = simple_strtol((*str), NULL, 0);
>>  inc_counter = upper_range - *pint;
>> -for (x = *pint; x < upper_range; x++)
>> +for (x = *pint; n && x < upper_range; x++, n--)
>>  *pint++ = x;
>>  return inc_counter;
>> }
> 
> But this still returns the number of integers in the range (minus 1)...
> 
>> @@ -96,7 +96,7 @@ char *get_options(const char *str, int n
>>  break;
>>  if (res == 3) {
>>  int range_nums;
>> -range_nums = get_range((char **), ints + i);
>> +range_nums = get_range((char **), ints + i, nints - 
>> i);
>>  if (range_nums < 0)
>>  break;
>>  /*
> 
> ...so that get_options() may set i > nints and ints[0] > nints - 1.
> That will presumably result in out-of-bounds reads in callers.
> 
> (This set of functions really deserves to be given a test suite and then
> rewritten, because they are a *mess*.)
> 

Please review the approach of fixing that:
https://lkml.org/lkml/2017/9/19/105

> Ben.
> 
> -- 
> Ben Hutchings
> Software Developer, Codethink Ltd.
> 
> 

Re: [PATCH 4.4 03/26] lib/cmdline.c: fix get_options() overflow while parsing ranges

[Ilya Matveychikov]

> On Jun 29, 2017, at 7:24 PM, Ben Hutchings  
> wrote:
> 
> On Tue, 2017-06-27 at 14:49 +0200, Greg Kroah-Hartman wrote:
>> 4.4-stable review patch.  If anyone has any objections, please let me know.
>> 
>> ------
>> 
>> From: Ilya Matveychikov 
>> 
>> commit a91e0f680bcd9e10c253ae8b62462a38bd48f09f upstream.
>> 
>> When using get_options() it's possible to specify a range of numbers,
>> like 1-100500.  The problem is that it doesn't track array size while
>> calling internally to get_range() which iterates over the range and
>> fills the memory with numbers.
> [...]
>> --- a/lib/cmdline.c
>> +++ b/lib/cmdline.c
>> @@ -22,14 +22,14 @@
>>  *   the values[M, M+1, ..., N] into the ints array in get_options.
>>  */
>> 
>> -static int get_range(char **str, int *pint)
>> +static int get_range(char **str, int *pint, int n)
>> {
>>  int x, inc_counter, upper_range;
>> 
>>  (*str)++;
>>  upper_range = simple_strtol((*str), NULL, 0);
>>  inc_counter = upper_range - *pint;
>> -for (x = *pint; x < upper_range; x++)
>> +for (x = *pint; n && x < upper_range; x++, n--)
>>  *pint++ = x;
>>  return inc_counter;
>> }
> 
> But this still returns the number of integers in the range (minus 1)...
> 
>> @@ -96,7 +96,7 @@ char *get_options(const char *str, int n
>>  break;
>>  if (res == 3) {
>>  int range_nums;
>> -range_nums = get_range((char **), ints + i);
>> +range_nums = get_range((char **), ints + i, nints - 
>> i);
>>  if (range_nums < 0)
>>  break;
>>  /*
> 
> ...so that get_options() may set i > nints and ints[0] > nints - 1.
> That will presumably result in out-of-bounds reads in callers.
> 
> (This set of functions really deserves to be given a test suite and then
> rewritten, because they are a *mess*.)
> 

Please review the approach of fixing that:
https://lkml.org/lkml/2017/9/19/105

> Ben.
> 
> -- 
> Ben Hutchings
> Software Developer, Codethink Ltd.
> 
> 

[RFC PATCH 2/3] net/dev/core.c: use ksa_parse_ints instead of get_options

[Ilya Matveychikov]

Signed-off-by: Ilya V. Matveychikov 
---
 net/core/dev.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8515f8f..acda9ac 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -144,6 +144,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "net-sysfs.h"

@@ -645,23 +646,18 @@ unsigned long netdev_boot_base(const char *prefix, int 
unit)
  */
 int __init netdev_boot_setup(char *str)
 {
-   int ints[5];
-   struct ifmap map;
+   struct ifmap map = { 0 };
+   KSA_DECLARE(ints, int, 4);

-   str = get_options(str, ARRAY_SIZE(ints), ints);
+   str = ksa_parse_ints(str, KSA());
if (!str || !*str)
return 0;

/* Save settings */
-   memset(, 0, sizeof(map));
-   if (ints[0] > 0)
-   map.irq = ints[1];
-   if (ints[0] > 1)
-   map.base_addr = ints[2];
-   if (ints[0] > 2)
-   map.mem_start = ints[3];
-   if (ints[0] > 3)
-   map.mem_end = ints[4];
+   map.irq = KSA_GET(, 0, 0);
+   map.base_addr = KSA_GET(, 1, 0);
+   map.mem_start = KSA_GET(, 2, 0);
+   map.mem_end = KSA_GET(, 3, 0);

/* Add new entry to the list */
return netdev_boot_setup_add(str, );
--
2.7.4

[RFC PATCH 2/3] net/dev/core.c: use ksa_parse_ints instead of get_options

[Ilya Matveychikov]

Signed-off-by: Ilya V. Matveychikov 
---
 net/core/dev.c | 20 
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 8515f8f..acda9ac 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -144,6 +144,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "net-sysfs.h"

@@ -645,23 +646,18 @@ unsigned long netdev_boot_base(const char *prefix, int 
unit)
  */
 int __init netdev_boot_setup(char *str)
 {
-   int ints[5];
-   struct ifmap map;
+   struct ifmap map = { 0 };
+   KSA_DECLARE(ints, int, 4);

-   str = get_options(str, ARRAY_SIZE(ints), ints);
+   str = ksa_parse_ints(str, KSA());
if (!str || !*str)
return 0;

/* Save settings */
-   memset(, 0, sizeof(map));
-   if (ints[0] > 0)
-   map.irq = ints[1];
-   if (ints[0] > 1)
-   map.base_addr = ints[2];
-   if (ints[0] > 2)
-   map.mem_start = ints[3];
-   if (ints[0] > 3)
-   map.mem_end = ints[4];
+   map.irq = KSA_GET(, 0, 0);
+   map.base_addr = KSA_GET(, 1, 0);
+   map.mem_start = KSA_GET(, 2, 0);
+   map.mem_end = KSA_GET(, 3, 0);

/* Add new entry to the list */
return netdev_boot_setup_add(str, );
--
2.7.4

[RFC PATCH 1/3] ksmall_array: introduce kernel small arrays

[Ilya Matveychikov]

Signed-off-by: Ilya V. Matveychikov 
---
 include/linux/small_array.h | 35 +++
 lib/Makefile|  2 +-
 lib/cmdline.c   |  4 +++-
 lib/ksmall_array.c  | 26 ++
 4 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/small_array.h
 create mode 100644 lib/ksmall_array.c

diff --git a/include/linux/small_array.h b/include/linux/small_array.h
new file mode 100644
index 000..c5ccbe4d
--- /dev/null
+++ b/include/linux/small_array.h
@@ -0,0 +1,35 @@
+#ifndef __SMALL_ARRAY_H__
+#define __SMALL_ARRAY_H__
+
+/*
+ * Kingdom of Small Arrays (KSA)
+ */
+
+#include 
+
+#define KSA_S_BITS 8
+#define KSA_N_BITS 12
+
+struct ksmall_array {
+   unsigned int s : KSA_S_BITS;/* sizeof(item) */
+   unsigned int n : KSA_N_BITS;/* number of items stored */
+   unsigned int m : KSA_N_BITS;/* maximum number of items allowed */
+   unsigned char v[0];
+};
+
+#define KSA_DECLARE(name, type, count) \
+   struct {\
+   struct ksmall_array ksa;\
+   type data[count];   \
+   } name = {{ .s = sizeof(type), .n = 0, .m = count }}
+
+#define KSA(p) ((struct ksmall_array *)(p))
+#define KSA_S(p)   KSA(p)->s
+#define KSA_N(p)   KSA(p)->n
+#define KSA_M(p)   KSA(p)->m
+#define KSA_V(p, t)((t *)KSA(p)->v)
+#define KSA_GET(p, i, v)   ((i < KSA_N(p)) ? (p)->data[i] : v)
+
+extern char *ksa_parse_ints(const char *str, struct ksmall_array *ksa);
+
+#endif /* __SMALL_ARRAY_H__ */
diff --git a/lib/Makefile b/lib/Makefile
index 40c1837..502f4b4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -30,7 +30,7 @@ lib-$(CONFIG_SMP) += cpumask.o
 lib-$(CONFIG_DMA_NOOP_OPS) += dma-noop.o
 lib-$(CONFIG_DMA_VIRT_OPS) += dma-virt.o

-lib-y  += kobject.o klist.o
+lib-y  += kobject.o klist.o ksmall_array.o
 obj-y  += lockref.o

 obj-y += bcd.o div64.o sort.o parser.o debug_locks.o random32.o \
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 4c0888c..233c4eb 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -63,8 +63,10 @@ int get_option(char **str, int *pint)
(*str)++;
return 2;
}
-   if (**str == '-')
+   if (**str == '-') {
+   (*str)++;
return 3;
+   }

return 1;
 }
diff --git a/lib/ksmall_array.c b/lib/ksmall_array.c
new file mode 100644
index 000..ecc8403
--- /dev/null
+++ b/lib/ksmall_array.c
@@ -0,0 +1,26 @@
+#include 
+#include 
+
+char *ksa_parse_ints(const char *str, struct ksmall_array *ksa)
+{
+   int res, a, b;
+
+   BUG_ON(ksa->s != sizeof(int));
+
+   while (ksa->n < ksa->m) {
+   res = get_option((char **), ); b = a;
+   if (res == 0) break;
+   if (res == 3) res = get_option((char **), );
+   if (res == 0) break;
+   while (a <= b && (ksa->n < ksa->m))
+   KSA_V(ksa, int)[ksa->n++] = a++;
+   }
+
+   return (char *)str;
+}
+EXPORT_SYMBOL(ksa_parse_ints);
+
+static void __attribute__((unused)) ksa_build_check(void)
+{
+   BUILD_BUG_ON(sizeof(struct ksmall_array) != sizeof(unsigned int));
+}
--
2.7.4

[RFC PATCH 1/3] ksmall_array: introduce kernel small arrays

[Ilya Matveychikov]

Signed-off-by: Ilya V. Matveychikov 
---
 include/linux/small_array.h | 35 +++
 lib/Makefile|  2 +-
 lib/cmdline.c   |  4 +++-
 lib/ksmall_array.c  | 26 ++
 4 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/small_array.h
 create mode 100644 lib/ksmall_array.c

diff --git a/include/linux/small_array.h b/include/linux/small_array.h
new file mode 100644
index 000..c5ccbe4d
--- /dev/null
+++ b/include/linux/small_array.h
@@ -0,0 +1,35 @@
+#ifndef __SMALL_ARRAY_H__
+#define __SMALL_ARRAY_H__
+
+/*
+ * Kingdom of Small Arrays (KSA)
+ */
+
+#include 
+
+#define KSA_S_BITS 8
+#define KSA_N_BITS 12
+
+struct ksmall_array {
+   unsigned int s : KSA_S_BITS;/* sizeof(item) */
+   unsigned int n : KSA_N_BITS;/* number of items stored */
+   unsigned int m : KSA_N_BITS;/* maximum number of items allowed */
+   unsigned char v[0];
+};
+
+#define KSA_DECLARE(name, type, count) \
+   struct {\
+   struct ksmall_array ksa;\
+   type data[count];   \
+   } name = {{ .s = sizeof(type), .n = 0, .m = count }}
+
+#define KSA(p) ((struct ksmall_array *)(p))
+#define KSA_S(p)   KSA(p)->s
+#define KSA_N(p)   KSA(p)->n
+#define KSA_M(p)   KSA(p)->m
+#define KSA_V(p, t)((t *)KSA(p)->v)
+#define KSA_GET(p, i, v)   ((i < KSA_N(p)) ? (p)->data[i] : v)
+
+extern char *ksa_parse_ints(const char *str, struct ksmall_array *ksa);
+
+#endif /* __SMALL_ARRAY_H__ */
diff --git a/lib/Makefile b/lib/Makefile
index 40c1837..502f4b4 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -30,7 +30,7 @@ lib-$(CONFIG_SMP) += cpumask.o
 lib-$(CONFIG_DMA_NOOP_OPS) += dma-noop.o
 lib-$(CONFIG_DMA_VIRT_OPS) += dma-virt.o

-lib-y  += kobject.o klist.o
+lib-y  += kobject.o klist.o ksmall_array.o
 obj-y  += lockref.o

 obj-y += bcd.o div64.o sort.o parser.o debug_locks.o random32.o \
diff --git a/lib/cmdline.c b/lib/cmdline.c
index 4c0888c..233c4eb 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -63,8 +63,10 @@ int get_option(char **str, int *pint)
(*str)++;
return 2;
}
-   if (**str == '-')
+   if (**str == '-') {
+   (*str)++;
return 3;
+   }

return 1;
 }
diff --git a/lib/ksmall_array.c b/lib/ksmall_array.c
new file mode 100644
index 000..ecc8403
--- /dev/null
+++ b/lib/ksmall_array.c
@@ -0,0 +1,26 @@
+#include 
+#include 
+
+char *ksa_parse_ints(const char *str, struct ksmall_array *ksa)
+{
+   int res, a, b;
+
+   BUG_ON(ksa->s != sizeof(int));
+
+   while (ksa->n < ksa->m) {
+   res = get_option((char **), ); b = a;
+   if (res == 0) break;
+   if (res == 3) res = get_option((char **), );
+   if (res == 0) break;
+   while (a <= b && (ksa->n < ksa->m))
+   KSA_V(ksa, int)[ksa->n++] = a++;
+   }
+
+   return (char *)str;
+}
+EXPORT_SYMBOL(ksa_parse_ints);
+
+static void __attribute__((unused)) ksa_build_check(void)
+{
+   BUILD_BUG_ON(sizeof(struct ksmall_array) != sizeof(unsigned int));
+}
--
2.7.4

[RFC PATCH 0/3] Introduce kernel small arrays (KSA)

[Ilya Matveychikov]

Hi guys,

Please review the approach of using small fixed-sized arrays to improve
parsing of values like get_options() does.

This comes to me after fixing an overflow in get_options(). See the thread
for details: https://lkml.org/lkml/2017/5/22/581

If the approach is OK I’ll suggest to replace all of get_options() calls
to ksa_parse_ints() and remove get_options() at all.

Thank you.

[RFC PATCH 0/3] Introduce kernel small arrays (KSA)

[Ilya Matveychikov]

Hi guys,

Please review the approach of using small fixed-sized arrays to improve
parsing of values like get_options() does.

This comes to me after fixing an overflow in get_options(). See the thread
for details: https://lkml.org/lkml/2017/5/22/581

If the approach is OK I’ll suggest to replace all of get_options() calls
to ksa_parse_ints() and remove get_options() at all.

Thank you.

Re: [PATCH] lib/cmdline.c: add to the get_options() documentation

[Ilya Matveychikov]

> On Aug 21, 2017, at 1:46 PM, Dan Carpenter  wrote:
> 
> I wasn't sure how get_options() worked, so I looked at examples.  And by
> sheer chance the first example I picked the only example which uses it
> incorrectly...  I've added some comments that hopefully help.
> 

See also comments on my patch from Ben Hutchings:
https://patchwork.kernel.org/patch/9811617/

Re: [PATCH] lib/cmdline.c: add to the get_options() documentation

[Ilya Matveychikov]

> On Aug 21, 2017, at 1:46 PM, Dan Carpenter  wrote:
> 
> I wasn't sure how get_options() worked, so I looked at examples.  And by
> sheer chance the first example I picked the only example which uses it
> incorrectly...  I've added some comments that hopefully help.
> 

See also comments on my patch from Ben Hutchings:
https://patchwork.kernel.org/patch/9811617/

[PATCH] cmdline: fix get_options() overflow while parsing ranges

[Ilya Matveychikov]

When using get_options() it's possible to specify a range of numbers,
like 1-100500. The problem is that it doesn't track array size while
calling internally to get_range() which iterates over the range and
fills the memory with numbers.

Signed-off-by: Ilya V. Matveychikov 
---
 lib/cmdline.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 8f13cf7..79069d7 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -22,14 +22,14 @@
  * the values[M, M+1, ..., N] into the ints array in get_options.
  */
 
-static int get_range(char **str, int *pint)
+static int get_range(char **str, int *pint, int n)
 {
int x, inc_counter, upper_range;
 
(*str)++;
upper_range = simple_strtol((*str), NULL, 0);
inc_counter = upper_range - *pint;
-   for (x = *pint; x < upper_range; x++)
+   for (x = *pint; n && x < upper_range; x++, n--)
*pint++ = x;
return inc_counter;
 }
@@ -96,7 +96,7 @@ char *get_options(const char *str, int nints, int *ints)
break;
if (res == 3) {
int range_nums;
-   range_nums = get_range((char **), ints + i);
+   range_nums = get_range((char **), ints + i, nints - 
i);
if (range_nums < 0)
break;
/*
-- 
2.7.4

[PATCH] cmdline: fix get_options() overflow while parsing ranges

[Ilya Matveychikov]

When using get_options() it's possible to specify a range of numbers,
like 1-100500. The problem is that it doesn't track array size while
calling internally to get_range() which iterates over the range and
fills the memory with numbers.

Signed-off-by: Ilya V. Matveychikov 
---
 lib/cmdline.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/cmdline.c b/lib/cmdline.c
index 8f13cf7..79069d7 100644
--- a/lib/cmdline.c
+++ b/lib/cmdline.c
@@ -22,14 +22,14 @@
  * the values[M, M+1, ..., N] into the ints array in get_options.
  */
 
-static int get_range(char **str, int *pint)
+static int get_range(char **str, int *pint, int n)
 {
int x, inc_counter, upper_range;
 
(*str)++;
upper_range = simple_strtol((*str), NULL, 0);
inc_counter = upper_range - *pint;
-   for (x = *pint; x < upper_range; x++)
+   for (x = *pint; n && x < upper_range; x++, n--)
*pint++ = x;
return inc_counter;
 }
@@ -96,7 +96,7 @@ char *get_options(const char *str, int nints, int *ints)
break;
if (res == 3) {
int range_nums;
-   range_nums = get_range((char **), ints + i);
+   range_nums = get_range((char **), ints + i, nints - 
i);
if (range_nums < 0)
break;
/*
-- 
2.7.4