In ll_dir_ioctl(), the object lumv3 is firstly copied from the user space
using Its address, i.e., lumv1 = &lumv3. If the lmm_magic field of lumv3 is
LOV_USER_MAGIC_V3, lumv3 will be modified by the second copy from the user
space. The second copy is necessary, because the two versions (i.e.,
lov_user_md_v1 and lov_user_md_v3) have different data formats and lengths.
However, given that the user data resides in the user space, a malicious
user-space process can race to change the data between the two copies. By
doing so, the attacker can provide a data with an inconsistent version,
e.g., v1 version + v3 data. This can lead to logical errors in the
following execution in ll_dir_setstripe(), which performs different actions
according to the version specified by the field lmm_magic.
This patch rechecks the version field lmm_magic in the second copy. If the
version is not as expected, i.e., LOV_USER_MAGIC_V3, an error code will be
returned: -EINVAL.
Signed-off-by: Wenwen Wang <[email protected]>
---
drivers/staging/lustre/lustre/llite/dir.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
index d10d272..80d44ca 100644
--- a/drivers/staging/lustre/lustre/llite/dir.c
+++ b/drivers/staging/lustre/lustre/llite/dir.c
@@ -1185,6 +1185,8 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
if (lumv1->lmm_magic == LOV_USER_MAGIC_V3) {
if (copy_from_user(&lumv3, lumv3p, sizeof(lumv3)))
return -EFAULT;
+ if (lumv3.lmm_magic != LOV_USER_MAGIC_V3)
+ return -EINVAL;
}
if (is_root_inode(inode))
--
2.7.4
On Apr 30, 2018, at 16:56, Wenwen Wang <[email protected]> wrote:
>
> In ll_dir_ioctl(), the object lumv3 is firstly copied from the user space
> using Its address, i.e., lumv1 = &lumv3. If the lmm_magic field of lumv3 is
> LOV_USER_MAGIC_V3, lumv3 will be modified by the second copy from the user
> space. The second copy is necessary, because the two versions (i.e.,
> lov_user_md_v1 and lov_user_md_v3) have different data formats and lengths.
> However, given that the user data resides in the user space, a malicious
> user-space process can race to change the data between the two copies. By
> doing so, the attacker can provide a data with an inconsistent version,
> e.g., v1 version + v3 data. This can lead to logical errors in the
> following execution in ll_dir_setstripe(), which performs different actions
> according to the version specified by the field lmm_magic.
>
> This patch rechecks the version field lmm_magic in the second copy. If the
> version is not as expected, i.e., LOV_USER_MAGIC_V3, an error code will be
> returned: -EINVAL.
>
> Signed-off-by: Wenwen Wang <[email protected]>
Thanks for the updated patch.
Reviewed-by: Andreas Dilger <[email protected]>
> ---
> drivers/staging/lustre/lustre/llite/dir.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/staging/lustre/lustre/llite/dir.c b/drivers/staging/lustre/lustre/llite/dir.c
> index d10d272..80d44ca 100644
> --- a/drivers/staging/lustre/lustre/llite/dir.c
> +++ b/drivers/staging/lustre/lustre/llite/dir.c
> @@ -1185,6 +1185,8 @@ static long ll_dir_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> if (lumv1->lmm_magic == LOV_USER_MAGIC_V3) {
> if (copy_from_user(&lumv3, lumv3p, sizeof(lumv3)))
> return -EFAULT;
> + if (lumv3.lmm_magic != LOV_USER_MAGIC_V3)
> + return -EINVAL;
> }
>
> if (is_root_inode(inode))
> --
> 2.7.4
>
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
On Mon, Apr 30, 2018 at 05:56:10PM -0500, Wenwen Wang wrote:
> However, given that the user data resides in the user space, a malicious
> user-space process can race to change the data between the two copies. By
> doing so, the attacker can provide a data with an inconsistent version,
> e.g., v1 version + v3 data. This can lead to logical errors in the
> following execution in ll_dir_setstripe(), which performs different actions
> according to the version specified by the field lmm_magic.
This part is misleading. The fix is to improve readability and make
static checkers happy. You're over dramatizing it to make people think
it has a security impact when it doesn't.
If the user wants to specify v1 data they can just say that on the first
read. They don't need to do funny tricks and race between the two
reads. It's allowed.
In other words this allows the user to do something in a very
complicated way which they are already allowed to do in a very simple
straight forward way.
regards,
dan carpenter
On Tue, May 1, 2018 at 3:46 AM, Dan Carpenter <[email protected]> wrote:
> On Mon, Apr 30, 2018 at 05:56:10PM -0500, Wenwen Wang wrote:
>> However, given that the user data resides in the user space, a malicious
>> user-space process can race to change the data between the two copies. By
>> doing so, the attacker can provide a data with an inconsistent version,
>> e.g., v1 version + v3 data. This can lead to logical errors in the
>> following execution in ll_dir_setstripe(), which performs different actions
>> according to the version specified by the field lmm_magic.
>
> This part is misleading. The fix is to improve readability and make
> static checkers happy. You're over dramatizing it to make people think
> it has a security impact when it doesn't.
>
> If the user wants to specify v1 data they can just say that on the first
> read. They don't need to do funny tricks and race between the two
> reads. It's allowed.
>
> In other words this allows the user to do something in a very
> complicated way which they are already allowed to do in a very simple
> straight forward way.
>
> regards,
> dan carpenter
Thanks for your comment, Dan! How about this:
However, given that the user data resides in the user space, a
malicious user-space process can race to change the data between the
two copies. By doing so, the attacker can provide a data with an
inconsistent version, e.g., v1 version + v3 data. The current kernel
can handle such inconsistent data. But, it may pose a potential
security risk for future implementations. Also, to improve code
readability and make static analysis tools happy, which will warn
about read-verify-re-read type bugs, this issue should be fixed.
Thanks,
Wenwen
There is no security problem here. The user is allowed to choose either
v1 or v3. Using a double read race condition to choose v1 is not
going to cause problems. It's slightly more complicated than just
choosing it directly but that doesn't make it a security issue.
It's a bit like typing with your feet in that just because using your
toes instead of your fingergs is more complicated, it doesn't make it a
security issue.
regards,
dan carpenter
On Fri, May 4, 2018 at 12:27 AM, Dan Carpenter <[email protected]> wrote:
> There is no security problem here. The user is allowed to choose either
> v1 or v3. Using a double read race condition to choose v1 is not
> going to cause problems. It's slightly more complicated than just
> choosing it directly but that doesn't make it a security issue.
>
> It's a bit like typing with your feet in that just because using your
> toes instead of your fingergs is more complicated, it doesn't make it a
> security issue.
>
> regards,
> dan carpenter
>
Thanks again for your comment, Dan! I revised the commit message and
removed the security risk:
However, given that the user data resides in the user space, a
malicious user-space process can race to change the data between the
two copies. By doing so, the user can provide a data with an
inconsistent version, e.g., v1 version + v3 data. To improve code
readability and make static analysis tools happy, which will warn
about read-verify-re-read type bugs, this issue should be fixed.
Thanks,
Wenwen
On May 3, 2018, at 22:19, Wenwen Wang <[email protected]> wrote:
>
> On Tue, May 1, 2018 at 3:46 AM, Dan Carpenter <[email protected]> wrote:
>> On Mon, Apr 30, 2018 at 05:56:10PM -0500, Wenwen Wang wrote:
>>> However, given that the user data resides in the user space, a malicious
>>> user-space process can race to change the data between the two copies. By
>>> doing so, the attacker can provide a data with an inconsistent version,
>>> e.g., v1 version + v3 data. This can lead to logical errors in the
>>> following execution in ll_dir_setstripe(), which performs different actions
>>> according to the version specified by the field lmm_magic.
>>
>> This part is misleading. The fix is to improve readability and make
>> static checkers happy. You're over dramatizing it to make people think
>> it has a security impact when it doesn't.
>>
>> If the user wants to specify v1 data they can just say that on the first
>> read. They don't need to do funny tricks and race between the two
>> reads. It's allowed.
>>
>> In other words this allows the user to do something in a very
>> complicated way which they are already allowed to do in a very simple
>> straight forward way.
>>
>> regards,
>> dan carpenter
>
> Thanks for your comment, Dan! How about this:
>
> However, given that the user data resides in the user space, a
> malicious user-space process can race to change the data between the
> two copies. By doing so, the attacker can provide a data with an
> inconsistent version, e.g., v1 version + v3 data. The current kernel
> can handle such inconsistent data. But, it may pose a potential
> security risk for future implementations. Also, to improve code
> readability and make static analysis tools happy, which will warn
> about read-verify-re-read type bugs, this issue should be fixed.
There is nothing preventing the user from using struct lov_mds_md_v3 but
filling in lmm_magic = LOV_MAGIC_V1 from the beginning, no need for a race.
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
On Fri, May 4, 2018 at 5:08 AM, Dilger, Andreas
<[email protected]> wrote:
> On May 3, 2018, at 22:19, Wenwen Wang <[email protected]> wrote:
>>
>> On Tue, May 1, 2018 at 3:46 AM, Dan Carpenter <[email protected]> wrote:
>>> On Mon, Apr 30, 2018 at 05:56:10PM -0500, Wenwen Wang wrote:
>>>> However, given that the user data resides in the user space, a malicious
>>>> user-space process can race to change the data between the two copies. By
>>>> doing so, the attacker can provide a data with an inconsistent version,
>>>> e.g., v1 version + v3 data. This can lead to logical errors in the
>>>> following execution in ll_dir_setstripe(), which performs different actions
>>>> according to the version specified by the field lmm_magic.
>>>
>>> This part is misleading. The fix is to improve readability and make
>>> static checkers happy. You're over dramatizing it to make people think
>>> it has a security impact when it doesn't.
>>>
>>> If the user wants to specify v1 data they can just say that on the first
>>> read. They don't need to do funny tricks and race between the two
>>> reads. It's allowed.
>>>
>>> In other words this allows the user to do something in a very
>>> complicated way which they are already allowed to do in a very simple
>>> straight forward way.
>>>
>>> regards,
>>> dan carpenter
>>
>> Thanks for your comment, Dan! How about this:
>>
>> However, given that the user data resides in the user space, a
>> malicious user-space process can race to change the data between the
>> two copies. By doing so, the attacker can provide a data with an
>> inconsistent version, e.g., v1 version + v3 data. The current kernel
>> can handle such inconsistent data. But, it may pose a potential
>> security risk for future implementations. Also, to improve code
>> readability and make static analysis tools happy, which will warn
>> about read-verify-re-read type bugs, this issue should be fixed.
>
> There is nothing preventing the user from using struct lov_mds_md_v3 but
> filling in lmm_magic = LOV_MAGIC_V1 from the beginning, no need for a race.
>
But, if the user uses such a struct, the second fetch won't be
executed. This is a little bit different from using LOV_MAGIC_V3
firstly and then changing it to LOV_MAGIC_V1.
Wenwen
On Fri, May 04, 2018 at 10:01:44AM -0500, Kangjie Lu wrote:
> > There is nothing preventing the user from using struct lov_mds_md_v3 but
> > filling in lmm_magic = LOV_MAGIC_V1 from the beginning, no need for a race.
> >
>
> Right. No need for users to race. There might be a type confusion issue
> though if V1
> object is used as V3 or the other way.
>
It's a bit confusing for someone reading the code, but in terms of the
kernel it's straightforward.
It's like if someone is typing with their toes, that's sort of confusing
but it's not a security issue. And here we're implementing a no typing
with your toes policy just to make things more higienic (static checkers
in this metaphor).
regards,
dan carpenter