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_MAGIV_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 27, 2018, at 17:45, Wenwen Wang <[email protected]> wrote:
> [PATCH] staging: luster: llite: fix potential missing-check bug when copying lumv
(typo) s/luster/lustre/
> 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_MAGIV_V3, lumv3 will be modified by the second copy from the user
(typo) s/MAGIV/MAGIC/
> 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 isn't a serious bug in the end. The LOV_USER_MAGIC_V3 check just copies
a bit more data from userspace (the lmm_pool field). It would be more of a
problem if the reverse was possible (copy smaller V1 buffer, but change the
magic to LOV_USER_MAGIC_V3 afterward), but this isn't possible since the second
copy is not done if there is a V1 magic. If the user changes from V3 magic
to V1 in a racy manner it means less data will be used than copied, which
is harmless.
> 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.
This isn't a bad idea in any case, since it verifies the data copied from
userspace is still valid.
Cheers, Andreas
> 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
>
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
> On Apr 27, 2018, at 17:45, Wenwen Wang <[email protected]> wrote:
> > [PATCH] staging: luster: llite: fix potential missing-check bug when copying lumv
>
> (typo) s/luster/lustre/
>
> > 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_MAGIV_V3, lumv3 will be modified by the second copy from the user
>
> (typo) s/MAGIV/MAGIC/
>
> > 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 isn't a serious bug in the end. The LOV_USER_MAGIC_V3 check just copies
> a bit more data from userspace (the lmm_pool field). It would be more of a
> problem if the reverse was possible (copy smaller V1 buffer, but change the
> magic to LOV_USER_MAGIC_V3 afterward), but this isn't possible since the second
> copy is not done if there is a V1 magic. If the user changes from V3 magic
> to V1 in a racy manner it means less data will be used than copied, which
> is harmless.
>
> > 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.
>
> This isn't a bad idea in any case, since it verifies the data copied from
> userspace is still valid.
So you agree with this patch? Or do not?
confused,
greg k-h
On Sun, Apr 29, 2018 at 8:20 AM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>> On Apr 27, 2018, at 17:45, Wenwen Wang <[email protected]> wrote:
>> > [PATCH] staging: luster: llite: fix potential missing-check bug when copying lumv
>>
>> (typo) s/luster/lustre/
>>
>> > 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_MAGIV_V3, lumv3 will be modified by the second copy from the user
>>
>> (typo) s/MAGIV/MAGIC/
>>
>> > 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 isn't a serious bug in the end. The LOV_USER_MAGIC_V3 check just copies
>> a bit more data from userspace (the lmm_pool field). It would be more of a
>> problem if the reverse was possible (copy smaller V1 buffer, but change the
>> magic to LOV_USER_MAGIC_V3 afterward), but this isn't possible since the second
>> copy is not done if there is a V1 magic. If the user changes from V3 magic
>> to V1 in a racy manner it means less data will be used than copied, which
>> is harmless.
>>
>> > 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.
>>
>> This isn't a bad idea in any case, since it verifies the data copied from
>> userspace is still valid.
>
> So you agree with this patch? Or do not?
>
> confused,
>
> greg k-h
It is worth fixing this bug, since it offers an opportunity for adversaries
to provide inconsistent user data. In addition to the unwanted version
LOV_USER_MAGIC_V1, a malicious user can also use the version
LMV_USER_MAGIC, which is also unexpected but allowed in the function
ll_dir_setstripe(). These inconsistent data can cause potential logical
errors in the following execution. Hence it is necessary to re-verify the
data copied from userspace.
Thanks!
Wenwen
On Sun, Apr 29, 2018 at 03:58:55PM -0500, Wenwen Wang wrote:
> It is worth fixing this bug, since it offers an opportunity for adversaries
> to provide inconsistent user data. In addition to the unwanted version
> LOV_USER_MAGIC_V1, a malicious user can also use the version
> LMV_USER_MAGIC, which is also unexpected but allowed in the function
> ll_dir_setstripe(). These inconsistent data can cause potential logical
> errors in the following execution. Hence it is necessary to re-verify the
> data copied from userspace.
>
This change doesn't really prevent any bugs in current kernels since
LMV_USER_MAGIC is the same thing as LOV_USER_MAGIC_V1 and the users are
allowed to use LOV_USER_MAGIC_V1 if they want.
But we should probably verify it just to make the code easier to read
and because there are static analysis tools which will warn about read
verify re-read type bugs.
regards,
dan carpenter
On Apr 29, 2018, at 07:20, Greg Kroah-Hartman <[email protected]> wrote:
>
> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>> On Apr 27, 2018, at 17:45, Wenwen Wang <[email protected]> wrote:
>>> [PATCH] staging: luster: llite: fix potential missing-check bug when copying lumv
>>
>> (typo) s/luster/lustre/
>>
>>> 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_MAGIV_V3, lumv3 will be modified by the second copy from the user
>>
>> (typo) s/MAGIV/MAGIC/
>>
>>> 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 isn't a serious bug in the end. The LOV_USER_MAGIC_V3 check just copies
>> a bit more data from userspace (the lmm_pool field). It would be more of a
>> problem if the reverse was possible (copy smaller V1 buffer, but change the
>> magic to LOV_USER_MAGIC_V3 afterward), but this isn't possible since the second
>> copy is not done if there is a V1 magic. If the user changes from V3 magic
>> to V1 in a racy manner it means less data will be used than copied, which
>> is harmless.
>>
>>> 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.
>>
>> This isn't a bad idea in any case, since it verifies the data copied from
>> userspace is still valid.
>
> So you agree with this patch? Or do not?
>
> confused,
I don't think it fixes a real bug, but it makes the code a bit more clear,
so I'm OK to land it (with minor corrections to commit message per above).
Cheers, Andreas
--
Andreas Dilger
Lustre Principal Architect
Intel Corporation
On Mon, Apr 30, 2018 at 5:38 PM, Dilger, Andreas
<[email protected]> wrote:
> On Apr 29, 2018, at 07:20, Greg Kroah-Hartman <[email protected]> wrote:
>>
>> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>>> On Apr 27, 2018, at 17:45, Wenwen Wang <[email protected]> wrote:
>>>> [PATCH] staging: luster: llite: fix potential missing-check bug when copying lumv
>>>
>>> (typo) s/luster/lustre/
>>>
>>>> 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_MAGIV_V3, lumv3 will be modified by the second copy from the user
>>>
>>> (typo) s/MAGIV/MAGIC/
>>>
>>>> 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 isn't a serious bug in the end. The LOV_USER_MAGIC_V3 check just copies
>>> a bit more data from userspace (the lmm_pool field). It would be more of a
>>> problem if the reverse was possible (copy smaller V1 buffer, but change the
>>> magic to LOV_USER_MAGIC_V3 afterward), but this isn't possible since the second
>>> copy is not done if there is a V1 magic. If the user changes from V3 magic
>>> to V1 in a racy manner it means less data will be used than copied, which
>>> is harmless.
>>>
>>>> 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.
>>>
>>> This isn't a bad idea in any case, since it verifies the data copied from
>>> userspace is still valid.
>>
>> So you agree with this patch? Or do not?
>>
>> confused,
>
> I don't think it fixes a real bug, but it makes the code a bit more clear,
> so I'm OK to land it (with minor corrections to commit message per above).
>
> Cheers, Andreas
> --
> Andreas Dilger
> Lustre Principal Architect
> Intel Corporation
>
Thanks! I will re-submit the patch with the corrected commit message.
Wenwen