LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
@ 2018-04-27 23:45 Wenwen Wang
  2018-04-28 16:04 ` Dilger, Andreas
  0 siblings, 1 reply; 7+ messages in thread
From: Wenwen Wang @ 2018-04-27 23:45 UTC (permalink / raw)
  To: wang6495
  Cc: kjlu, Oleg Drokin, Andreas Dilger, James Simmons,
	Greg Kroah-Hartman, Ben Evans, Aastha Gupta, NeilBrown,
	Jeff Layton, Luis de Bethencourt, lustre-devel, devel,
	linux-kernel

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 <wang6495@umn.edu>
---
 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-27 23:45 [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv Wenwen Wang
@ 2018-04-28 16:04 ` Dilger, Andreas
  2018-04-29 13:20   ` Greg Kroah-Hartman
  0 siblings, 1 reply; 7+ messages in thread
From: Dilger, Andreas @ 2018-04-28 16:04 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: devel, Ben Evans, Jeff Layton, Aastha Gupta, Greg Kroah-Hartman,
	kjlu, NeilBrown, linux-kernel, Drokin, Oleg, lustre-devel

On Apr 27, 2018, at 17:45, Wenwen Wang <wang6495@umn.edu> 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 <wang6495@umn.edu>
> ---
> 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







_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-28 16:04 ` Dilger, Andreas
@ 2018-04-29 13:20   ` Greg Kroah-Hartman
  2018-04-29 20:58     ` Wenwen Wang
  2018-04-30 22:38     ` Dilger, Andreas
  0 siblings, 2 replies; 7+ messages in thread
From: Greg Kroah-Hartman @ 2018-04-29 13:20 UTC (permalink / raw)
  To: Dilger, Andreas
  Cc: Wenwen Wang, devel, Ben Evans, Jeff Layton, Aastha Gupta, kjlu,
	NeilBrown, linux-kernel, Drokin, Oleg, lustre-devel

On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
> On Apr 27, 2018, at 17:45, Wenwen Wang <wang6495@umn.edu> 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-29 13:20   ` Greg Kroah-Hartman
@ 2018-04-29 20:58     ` Wenwen Wang
  2018-04-30 11:15       ` Dan Carpenter
  2018-04-30 22:38     ` Dilger, Andreas
  1 sibling, 1 reply; 7+ messages in thread
From: Wenwen Wang @ 2018-04-29 20:58 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devel, Aastha Gupta, Dilger, Andreas, Jeff Layton, Drokin, Oleg,
	Wenwen Wang, kjlu, NeilBrown, linux-kernel, Ben Evans,
	lustre-devel

On Sun, Apr 29, 2018 at 8:20 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>> On Apr 27, 2018, at 17:45, Wenwen Wang <wang6495@umn.edu> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-29 20:58     ` Wenwen Wang
@ 2018-04-30 11:15       ` Dan Carpenter
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2018-04-30 11:15 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Greg Kroah-Hartman, devel, Aastha Gupta, Dilger, Andreas,
	Jeff Layton, Drokin, Oleg, kjlu, NeilBrown, linux-kernel,
	Ben Evans, lustre-devel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-29 13:20   ` Greg Kroah-Hartman
  2018-04-29 20:58     ` Wenwen Wang
@ 2018-04-30 22:38     ` Dilger, Andreas
  2018-04-30 22:43       ` Wenwen Wang
  1 sibling, 1 reply; 7+ messages in thread
From: Dilger, Andreas @ 2018-04-30 22:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devel, Aastha Gupta, Jeff Layton, Drokin, Oleg, Wenwen Wang,
	kjlu, NeilBrown, linux-kernel, Ben Evans, lustre-devel

On Apr 29, 2018, at 07:20, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>> On Apr 27, 2018, at 17:45, Wenwen Wang <wang6495@umn.edu> 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







_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv
  2018-04-30 22:38     ` Dilger, Andreas
@ 2018-04-30 22:43       ` Wenwen Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Wenwen Wang @ 2018-04-30 22:43 UTC (permalink / raw)
  To: Dilger, Andreas
  Cc: devel, Aastha Gupta, Jeff Layton, Drokin, Oleg,
	Greg Kroah-Hartman, Wenwen Wang, kjlu, NeilBrown, linux-kernel,
	Ben Evans, lustre-devel

On Mon, Apr 30, 2018 at 5:38 PM, Dilger, Andreas
<andreas.dilger@intel.com> wrote:
> On Apr 29, 2018, at 07:20, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>
>> On Sat, Apr 28, 2018 at 04:04:25PM +0000, Dilger, Andreas wrote:
>>> On Apr 27, 2018, at 17:45, Wenwen Wang <wang6495@umn.edu> 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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-04-30 22:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-27 23:45 [PATCH] staging: luster: llite: fix a potential missing-check bug when copying lumv Wenwen Wang
2018-04-28 16:04 ` Dilger, Andreas
2018-04-29 13:20   ` Greg Kroah-Hartman
2018-04-29 20:58     ` Wenwen Wang
2018-04-30 11:15       ` Dan Carpenter
2018-04-30 22:38     ` Dilger, Andreas
2018-04-30 22:43       ` Wenwen Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).