diff mbox series

[SRU,Focal,Focal/oem-5.6] xfs: fix boundary test in xfs_attr_shortform_verify

Message ID 20200916175015.392204-1-cascardo@canonical.com
State New
Headers show
Series [SRU,Focal,Focal/oem-5.6] xfs: fix boundary test in xfs_attr_shortform_verify | expand

Commit Message

Thadeu Lima de Souza Cascardo Sept. 16, 2020, 5:50 p.m. UTC 
From: Eric Sandeen <sandeen@redhat.com>

The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
xfs_attr_shortform_verify is off by one, because the variable array
at the end is defined as nameval[1] not nameval[].
Hence we need to subtract 1 from the calculation.

This can be shown by:

and verifications will fail when it's written to disk.

This only matters for a last attribute which has a single-byte name
and no value, otherwise the combination of namelen & valuelen will
push endp further out and this test won't fail.

Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
(cherry picked from commit f4020438fab05364018c91f7e02ebdd192085933)
CVE-2020-14385
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Stefan Bader Sept. 17, 2020, 7:39 a.m. UTC | #1 
On 16.09.20 19:50, Thadeu Lima de Souza Cascardo wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
> xfs_attr_shortform_verify is off by one, because the variable array
> at the end is defined as nameval[1] not nameval[].
> Hence we need to subtract 1 from the calculation.
> 
> This can be shown by:
> 
> and verifications will fail when it's written to disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp further out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> (cherry picked from commit f4020438fab05364018c91f7e02ebdd192085933)
> CVE-2020-14385
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fed537a4353d..54c412549edf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1010,8 +1010,10 @@ xfs_attr_shortform_verify(
>  		 * struct xfs_attr_sf_entry has a variable length.
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
> +		 * xfs_attr_sf_entry is defined with a 1-byte variable
> +		 * array at the end, so we must subtract that off.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
>
Colin Ian King Sept. 17, 2020, 8:33 a.m. UTC | #2 
On 16/09/2020 18:50, Thadeu Lima de Souza Cascardo wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
> xfs_attr_shortform_verify is off by one, because the variable array
> at the end is defined as nameval[1] not nameval[].
> Hence we need to subtract 1 from the calculation.
> 
> This can be shown by:
> 
> and verifications will fail when it's written to disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp further out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> (cherry picked from commit f4020438fab05364018c91f7e02ebdd192085933)
> CVE-2020-14385
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fed537a4353d..54c412549edf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1010,8 +1010,10 @@ xfs_attr_shortform_verify(
>  		 * struct xfs_attr_sf_entry has a variable length.
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
> +		 * xfs_attr_sf_entry is defined with a 1-byte variable
> +		 * array at the end, so we must subtract that off.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
> 

Clean cherry pick, looks good to me.

Acked-by: Colin Ian King <colin.king@canonical.com>
Ian May Sept. 17, 2020, 11:27 p.m. UTC | #3 
This patch was applied in the following patchset:

Focal update: v5.4.64 upstream stable release
https://bugs.launchpad.net/bugs/1895880

Thanks!
Ian

On 2020-09-16 14:50:15 , Thadeu Lima de Souza Cascardo wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
> xfs_attr_shortform_verify is off by one, because the variable array
> at the end is defined as nameval[1] not nameval[].
> Hence we need to subtract 1 from the calculation.
> 
> This can be shown by:
> 
> and verifications will fail when it's written to disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp further out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> (cherry picked from commit f4020438fab05364018c91f7e02ebdd192085933)
> CVE-2020-14385
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fed537a4353d..54c412549edf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1010,8 +1010,10 @@ xfs_attr_shortform_verify(
>  		 * struct xfs_attr_sf_entry has a variable length.
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
> +		 * xfs_attr_sf_entry is defined with a 1-byte variable
> +		 * array at the end, so we must subtract that off.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
> -- 
> 2.25.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Timo Aaltonen Sept. 22, 2020, 12:17 p.m. UTC | #4 
On 16.9.2020 20.50, Thadeu Lima de Souza Cascardo wrote:
> From: Eric Sandeen <sandeen@redhat.com>
> 
> The boundary test for the fixed-offset parts of xfs_attr_sf_entry in
> xfs_attr_shortform_verify is off by one, because the variable array
> at the end is defined as nameval[1] not nameval[].
> Hence we need to subtract 1 from the calculation.
> 
> This can be shown by:
> 
> and verifications will fail when it's written to disk.
> 
> This only matters for a last attribute which has a single-byte name
> and no value, otherwise the combination of namelen & valuelen will
> push endp further out and this test won't fail.
> 
> Fixes: 1e1bbd8e7ee06 ("xfs: create structure verifier function for shortform xattrs")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> (cherry picked from commit f4020438fab05364018c91f7e02ebdd192085933)
> CVE-2020-14385
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index fed537a4353d..54c412549edf 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -1010,8 +1010,10 @@ xfs_attr_shortform_verify(
>  		 * struct xfs_attr_sf_entry has a variable length.
>  		 * Check the fixed-offset parts of the structure are
>  		 * within the data buffer.
> +		 * xfs_attr_sf_entry is defined with a 1-byte variable
> +		 * array at the end, so we must subtract that off.
>  		 */
> -		if (((char *)sfep + sizeof(*sfep)) >= endp)
> +		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
>  			return __this_address;
>  
>  		/* Don't allow names with known bad length. */
> 

applied to oem-5.6, thanks
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index fed537a4353d..54c412549edf 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -1010,8 +1010,10 @@  xfs_attr_shortform_verify(
 		 * struct xfs_attr_sf_entry has a variable length.
 		 * Check the fixed-offset parts of the structure are
 		 * within the data buffer.
+		 * xfs_attr_sf_entry is defined with a 1-byte variable
+		 * array at the end, so we must subtract that off.
 		 */
-		if (((char *)sfep + sizeof(*sfep)) >= endp)
+		if (((char *)sfep + sizeof(*sfep) - 1) >= endp)
 			return __this_address;
 
 		/* Don't allow names with known bad length. */