Re: arch/s390/kvm/gaccess.c:859 guest_range_to_gpas() error: uninitialized symbol 'prot'.

[Claudio Imbrenda]

On Wed, 17 Aug 2022 12:30:11 +0200
Janis Schoetterl-Glausch <scgl@xxxxxxxxxxxxx> wrote:

> On 8/15/22 09:23, Dan Carpenter wrote:
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   69dac8e431af26173ca0a1ebc87054e01c585bcc
> > commit: 7faa543df19bf62d4583a64d3902705747f2ad29 KVM: s390: gaccess: Refactor access address range check
> > config: s390-randconfig-m031-20220812 (https://download.01.org/0day-ci/archive/20220814/202208140141.4uzWszG4-lkp@xxxxxxxxx/config)
> > compiler: s390-linux-gcc (GCC) 12.1.0
> > 
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > 
> > smatch warnings:
> > arch/s390/kvm/gaccess.c:859 guest_range_to_gpas() error: uninitialized symbol 'prot'.
> > 
> > vim +/prot +859 arch/s390/kvm/gaccess.c
> > 
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  831  static int guest_range_to_gpas(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  832  			       unsigned long *gpas, unsigned long len,
> > 92c9632119b67f David Hildenbrand        2015-11-16  833  			       const union asce asce, enum gacc_mode mode)
> > 2293897805c2fe Heiko Carstens           2014-01-01  834  {
> > 2293897805c2fe Heiko Carstens           2014-01-01  835  	psw_t *psw = &vcpu->arch.sie_block->gpsw;
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  836  	unsigned int offset = offset_in_page(ga);
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  837  	unsigned int fragment_len;
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  838  	int lap_enabled, rc = 0;
> > 6ae1574c2a24ee Christian Borntraeger    2017-06-07  839  	enum prot_type prot;
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  840  	unsigned long gpa;
> > 2293897805c2fe Heiko Carstens           2014-01-01  841  
> > 75a1812230ad7a Alexander Yarygin        2015-01-22  842  	lap_enabled = low_address_protection_enabled(vcpu, asce);
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  843  	while (min(PAGE_SIZE - offset, len) > 0) {
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  844  		fragment_len = min(PAGE_SIZE - offset, len);
> > 2293897805c2fe Heiko Carstens           2014-01-01  845  		ga = kvm_s390_logical_to_effective(vcpu, ga);
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  846  		if (mode == GACC_STORE && lap_enabled && is_low_address(ga))
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  847  			return trans_exc(vcpu, PGM_PROTECTION, ga, ar, mode,
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  848  					 PROT_TYPE_LA);
> > a752598254016d Heiko Carstens           2017-06-03  849  		if (psw_bits(*psw).dat) {
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  850  			rc = guest_translate(vcpu, ga, &gpa, asce, mode, &prot);
> > 2293897805c2fe Heiko Carstens           2014-01-01  851  			if (rc < 0)
> > 2293897805c2fe Heiko Carstens           2014-01-01  852  				return rc;
> > 2293897805c2fe Heiko Carstens           2014-01-01  853  		} else {
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  854  			gpa = kvm_s390_real_to_abs(vcpu, ga);
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  855  			if (kvm_is_error_gpa(vcpu->kvm, gpa))
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  856  				rc = PGM_ADDRESSING;
> >                                                                                         ^^^^^^^^^^^^^^^^^^^^
> > 
> > "rc" set but "prot" not initialized.  
> 
> prot is only used in case of PGM_PROTECTION.
> Because of that, we could initialize prot to an arbitrary value, but that seems misleading and a bit ugly to me.
> Alternatively, we could introduce a PROT_NONE.
> Or we do nothing, since there is no actual problem.
> @Janosch, @Claudio, what do you think?

I agree that this is not a bug.

It does look ugly, though, and all reasonable solutions are also ugly
(each for a different reason).

another solution is to set prot to an arbitrary value only in the if
case marked above. that way prot is not arbitrarily initialized, and
there is no need to add a new PROT_NONE (which then would need to be
checked for in trans_exc)

I do not have a strong opinion, though

> 
> > 
> > 2293897805c2fe Heiko Carstens           2014-01-01  857  		}
> > cde0dcfb5df1db David Hildenbrand        2016-05-31  858  		if (rc)
> > 6ae1574c2a24ee Christian Borntraeger    2017-06-07 @859  			return trans_exc(vcpu, rc, ga, ar, mode, prot);
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  860  		if (gpas)
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  861  			*gpas++ = gpa;
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  862  		offset = 0;
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  863  		ga += fragment_len;
> > 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26  864  		len -= fragment_len;
> > 2293897805c2fe Heiko Carstens           2014-01-01  865  	}
> > 2293897805c2fe Heiko Carstens           2014-01-01  866  	return 0;
> > 2293897805c2fe Heiko Carstens           2014-01-01  867  }
> >   
>