Skip to contentSkip to conversation

orangefs: off by ones in xattr size checks

PATCH In review Dan Carpenter 2 participants 2 messages May 22, 2017

Subsystems (2)
Kernel janitors Orangefs filesystem
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 orangefs: off by ones in xattr size checks

A previous patch which claimed to remove off by ones actually introduced them.

strlen() returns the length of the string not including the NUL character. We are using strcpy() to copy "name" into a buffer which is ORANGEFS_MAX_XATTR_NAMELEN characters long. We should make sure to leave space for the NUL, otherwise we're writing one character beyond the end of the buffer.

Fixes e675c5ec51fe ("orangefs: clean up oversize xattr validation")
Signed-off-by Dan Carpenter <[email protected]>
fs/orangefs/xattr.c +3 -3
index 237c9c04dc3b..a34b25be39c5 100644
@@ -76,7 +76,7 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const char *name,
 	if (S_ISLNK(inode->i_mode))
 		return -EOPNOTSUPP;
 
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;
 
 	fsuid = from_kuid(&init_user_ns, current_fsuid());

@@ -169,7 +169,7 @@ static int orangefs_inode_removexattr(struct inode *inode, const char *name,
 	struct orangefs_kernel_op_s *new_op = NULL;
 	int ret = -ENOMEM;
 
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;
 
 	down_write(&orangefs_inode->xattr_sem);

@@ -233,7 +233,7 @@ int orangefs_inode_setxattr(struct inode *inode, const char *name,
 
 	if (size > ORANGEFS_MAX_XATTR_VALUELEN)
 		return -EINVAL;
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;
 
 	internal_flag = convert_to_internal_xattr_flags(flags);
reviewer Reviewed-by on v1 1/1
#1 lore raw
Quoted 11 lines · Dan Carpenter

A previous patch which claimed to remove off by ones actually introduced them.

strlen() returns the length of the string not including the NUL character. We are using strcpy() to copy "name" into a buffer which is ORANGEFS_MAX_XATTR_NAMELEN characters long. We should make sure to leave space for the NUL, otherwise we're writing one character beyond the end of the buffer.

Fixes e675c5ec51fe ("orangefs: clean up oversize xattr validation")
Signed-off-by Dan Carpenter <[email protected]>

Crap. This is right. The OrangeFS userspace tools and the kernel both pass the zero byte. The server itself and our database is perfectly capable of handling any byte array. The value can be passed unchanged though (and you have left that). We still pass xfstests generic/020 with this applied.

Reviewed-by Martin Brandenburg <[email protected]>
Quoted 36 lines
fs/orangefs/xattr.c +1 -1
index 237c9c04dc3b..a34b25be39c5 100644
@@ -76,7 +76,7 @@ ssize_t orangefs_inode_getxattr(struct inode *inode, const
 char *name,
 	if (S_ISLNK(inode->i_mode))
 		return -EOPNOTSUPP;
 
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;
fs/orangefs/xattr.c +1 -1
 	fsuid = from_kuid(&init_user_ns, current_fsuid());

@@ -169,7 +169,7 @@ static int orangefs_inode_removexattr(struct inode
 *inode, const char *name,
 	struct orangefs_kernel_op_s *new_op = NULL;
 	int ret = -ENOMEM;
 
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;
fs/orangefs/xattr.c +1 -1
 	down_write(&orangefs_inode->xattr_sem);

@@ -233,7 +233,7 @@ int orangefs_inode_setxattr(struct inode *inode, const
 char *name,
 
 	if (size > ORANGEFS_MAX_XATTR_VALUELEN)
 		return -EINVAL;
-	if (strlen(name) > ORANGEFS_MAX_XATTR_NAMELEN)
+	if (strlen(name) >= ORANGEFS_MAX_XATTR_NAMELEN)
 		return -EINVAL;

internal_flag = convert_to_internal_xattr_flags(flags);