arm64: cpufeature: Fix truncating a feature value

Message ID 20191010131943.26822-1-suzuki.poulose@arm.com
State New
Headers show
Series
  • arm64: cpufeature: Fix truncating a feature value
Related show

Commit Message

Suzuki Kuruppassery Poulose Oct. 10, 2019, 1:19 p.m.
A signed feature value is truncated to turn to an unsigned value
causing bad state in the system wide infrastructure. This affects
the discovery of FP/ASIMD support on arm64. Fix this by making sure
we cast it properly.

This was inadvertently fixed upstream in v4.6 onwards with the following :
commit 28c5dcb22f90113dea ("arm64: Rename cpuid_feature field extract routines")

Cc: stable@vger.kernel.org # v4.4
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

---
 arch/arm64/include/asm/cpufeature.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.21.0

Comments

Greg KH Oct. 11, 2019, 4:55 a.m. | #1
On Thu, Oct 10, 2019 at 02:19:43PM +0100, Suzuki K Poulose wrote:
> A signed feature value is truncated to turn to an unsigned value

> causing bad state in the system wide infrastructure. This affects

> the discovery of FP/ASIMD support on arm64. Fix this by making sure

> we cast it properly.

> 

> This was inadvertently fixed upstream in v4.6 onwards with the following :

> commit 28c5dcb22f90113dea ("arm64: Rename cpuid_feature field extract routines")


What prevents us from just taking that commit instead?  You did not
document that here at all, which I thought I asked for.

Also, you only need 12 digits for a sha1, 28c5dcb22f90 ("arm64: Rename
cpuid_feature field extract routines") would be just fine :)

thanks,

greg k-h
Suzuki Kuruppassery Poulose Oct. 11, 2019, 10:31 a.m. | #2
Hi Greg,

On 11/10/2019 05:55, Greg KH wrote:
> On Thu, Oct 10, 2019 at 02:19:43PM +0100, Suzuki K Poulose wrote:

>> A signed feature value is truncated to turn to an unsigned value

>> causing bad state in the system wide infrastructure. This affects

>> the discovery of FP/ASIMD support on arm64. Fix this by making sure

>> we cast it properly.

>>

>> This was inadvertently fixed upstream in v4.6 onwards with the following :

>> commit 28c5dcb22f90113dea ("arm64: Rename cpuid_feature field extract routines")

> 

> What prevents us from just taking that commit instead?  You did not

> document that here at all, which I thought I asked for.


Sorry, I missed that part. So, that change introduces helpers to
extract feature fields based on the sign. And it also depends on

commit ff96f7bc7bf6 ("arm64: capabilities: Handle sign of the feature bit")

which introduces "sign" bit for the "capability" list and modifies
the generic capability->matches() helpers to use the hint to switch to the
appropriate helpers.

I could backport parts of the commit 28c5dcb22f90 dropping the bits
that affect the changes mentioned above.

> 

> Also, you only need 12 digits for a sha1, 28c5dcb22f90 ("arm64: Rename

> cpuid_feature field extract routines") would be just fine :)


Yea, I understand. Its simply a pain to count the numbers, so I make sure
to pickup something that looks larger than the 12 ;-). I will try to stick
to that :-)

Cheers
Suzuki
Greg KH Oct. 11, 2019, 12:13 p.m. | #3
On Fri, Oct 11, 2019 at 11:31:30AM +0100, Suzuki K Poulose wrote:
> Hi Greg,

> 

> On 11/10/2019 05:55, Greg KH wrote:

> > On Thu, Oct 10, 2019 at 02:19:43PM +0100, Suzuki K Poulose wrote:

> > > A signed feature value is truncated to turn to an unsigned value

> > > causing bad state in the system wide infrastructure. This affects

> > > the discovery of FP/ASIMD support on arm64. Fix this by making sure

> > > we cast it properly.

> > > 

> > > This was inadvertently fixed upstream in v4.6 onwards with the following :

> > > commit 28c5dcb22f90113dea ("arm64: Rename cpuid_feature field extract routines")

> > 

> > What prevents us from just taking that commit instead?  You did not

> > document that here at all, which I thought I asked for.

> 

> Sorry, I missed that part. So, that change introduces helpers to

> extract feature fields based on the sign. And it also depends on

> 

> commit ff96f7bc7bf6 ("arm64: capabilities: Handle sign of the feature bit")

> 

> which introduces "sign" bit for the "capability" list and modifies

> the generic capability->matches() helpers to use the hint to switch to the

> appropriate helpers.


That's ok, does that cause any problems?  We always want the original
patch instead of a one-off patch as that way we do not diverge.

> I could backport parts of the commit 28c5dcb22f90 dropping the bits

> that affect the changes mentioned above.


Please do, that is always prefered as well, but do the first thing above
if at all possible.

> > 

> > Also, you only need 12 digits for a sha1, 28c5dcb22f90 ("arm64: Rename

> > cpuid_feature field extract routines") would be just fine :)

> 

> Yea, I understand. Its simply a pain to count the numbers, so I make sure

> to pickup something that looks larger than the 12 ;-). I will try to stick

> to that :-)


	git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n"
will give you the correct format.  I suggest making it a git alias :)

Or, use:
	[core]
	        abbrev = 12
in your .gitconfig file.

thanks,

greg k-h

Patch

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 0a66f8241f18..9eb0d8072dd9 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -151,8 +151,8 @@  static inline u64 arm64_ftr_mask(struct arm64_ftr_bits *ftrp)
 static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
 {
 	return ftrp->sign ?
-		cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
-		cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
+		(s64)cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width) :
+		(s64)cpuid_feature_extract_unsigned_field_width(val, ftrp->shift, ftrp->width);
 }
 
 static inline bool id_aa64mmfr0_mixed_endian_el0(u64 mmfr0)