| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 19 Apr 2022 11:05:50 +0200
From: Janis Schoetterl-Glausch <scgl@...ux.ibm.com>
To: Dan Carpenter <dan.carpenter@...cle.com>, kbuild@...ts.01.org
Cc: lkp@...el.com, kbuild-all@...ts.01.org,
linux-kernel@...r.kernel.org,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>
Subject: Re: arch/s390/kvm/gaccess.c:1064 access_guest_with_key() error:
uninitialized symbol 'prot'.
On 4/15/22 11:30, Dan Carpenter wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 028192fea1de083f4f12bfb1eb7c4d7beb5c8ecd
> commit: e613d83454d7da1c37d78edb278db9c20afb21a2 KVM: s390: Honor storage keys when accessing guest memory
> config: s390-randconfig-m031-20220414 (https://download.01.org/0day-ci/archive/20220415/202204151626.pDKgwv97-lkp@intel.com/config)
> compiler: s390-linux-gcc (GCC) 11.2.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@...el.com>
> Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
>
> New smatch warnings:
> arch/s390/kvm/gaccess.c:1064 access_guest_with_key() error: uninitialized symbol 'prot'.
>
> Old smatch warnings:
> arch/s390/kvm/gaccess.c:935 guest_range_to_gpas() error: uninitialized symbol 'prot'.
>
> vim +/prot +1064 arch/s390/kvm/gaccess.c
>
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 997 int access_guest_with_key(struct kvm_vcpu *vcpu, unsigned long ga, u8 ar,
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 998 void *data, unsigned long len, enum gacc_mode mode,
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 999 u8 access_key)
> 2293897805c2fe Heiko Carstens 2014-01-01 1000 {
> 2293897805c2fe Heiko Carstens 2014-01-01 1001 psw_t *psw = &vcpu->arch.sie_block->gpsw;
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1002 unsigned long nr_pages, idx;
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1003 unsigned long gpa_array[2];
> 416e7f0c9d613b Janis Schoetterl-Glausch 2021-11-26 1004 unsigned int fragment_len;
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1005 unsigned long *gpas;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1006 enum prot_type prot;
>
> Maybe set "prot" to a default value?>
> 8a242234b4bfed Heiko Carstens 2014-01-10 1007 int need_ipte_lock;
> 8a242234b4bfed Heiko Carstens 2014-01-10 1008 union asce asce;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1009 bool try_storage_prot_override;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1010 bool try_fetch_prot_override;
> 2293897805c2fe Heiko Carstens 2014-01-01 1011 int rc;
> 2293897805c2fe Heiko Carstens 2014-01-01 1012
> 2293897805c2fe Heiko Carstens 2014-01-01 1013 if (!len)
> 2293897805c2fe Heiko Carstens 2014-01-01 1014 return 0;
> 6167375b558196 David Hildenbrand 2016-05-31 1015 ga = kvm_s390_logical_to_effective(vcpu, ga);
> 6167375b558196 David Hildenbrand 2016-05-31 1016 rc = get_vcpu_asce(vcpu, &asce, ga, ar, mode);
> 664b4973537068 Alexander Yarygin 2015-03-09 1017 if (rc)
> 664b4973537068 Alexander Yarygin 2015-03-09 1018 return rc;
> 2293897805c2fe Heiko Carstens 2014-01-01 1019 nr_pages = (((ga & ~PAGE_MASK) + len - 1) >> PAGE_SHIFT) + 1;
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1020 gpas = gpa_array;
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1021 if (nr_pages > ARRAY_SIZE(gpa_array))
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1022 gpas = vmalloc(array_size(nr_pages, sizeof(unsigned long)));
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1023 if (!gpas)
> 2293897805c2fe Heiko Carstens 2014-01-01 1024 return -ENOMEM;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1025 try_fetch_prot_override = fetch_prot_override_applicable(vcpu, mode, asce);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1026 try_storage_prot_override = storage_prot_override_applicable(vcpu);
> a752598254016d Heiko Carstens 2017-06-03 1027 need_ipte_lock = psw_bits(*psw).dat && !asce.r;
> 8a242234b4bfed Heiko Carstens 2014-01-10 1028 if (need_ipte_lock)
> 8a242234b4bfed Heiko Carstens 2014-01-10 1029 ipte_lock(vcpu);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1030 /*
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1031 * Since we do the access further down ultimately via a move instruction
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1032 * that does key checking and returns an error in case of a protection
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1033 * violation, we don't need to do the check during address translation.
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1034 * Skip it by passing access key 0, which matches any storage key,
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1035 * obviating the need for any further checks. As a result the check is
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1036 * handled entirely in hardware on access, we only need to take care to
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1037 * forego key protection checking if fetch protection override applies or
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1038 * retry with the special key 9 in case of storage protection override.
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1039 */
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1040 rc = guest_range_to_gpas(vcpu, ga, ar, gpas, len, asce, mode, 0);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1041 if (rc)
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1042 goto out_unlock;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1043 for (idx = 0; idx < nr_pages; idx++) {
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1044 fragment_len = min(PAGE_SIZE - offset_in_page(gpas[idx]), len);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1045 if (try_fetch_prot_override && fetch_prot_override_applies(ga, fragment_len)) {
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1046 rc = access_guest_page(vcpu->kvm, mode, gpas[idx],
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1047 data, fragment_len);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1048 } else {
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1049 rc = access_guest_page_with_key(vcpu->kvm, mode, gpas[idx],
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1050 data, fragment_len, access_key);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1051 }
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1052 if (rc == PGM_PROTECTION && try_storage_prot_override)
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1053 rc = access_guest_page_with_key(vcpu->kvm, mode, gpas[idx],
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1054 data, fragment_len, PAGE_SPO_ACC);
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1055 if (rc == PGM_PROTECTION)
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1056 prot = PROT_TYPE_KEYC;
>
> Is PGM_PROTECTION the only positive value that "rc" can be?
No, PGM_ADDRESSING is also possible.
>
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1057 if (rc)
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1058 break;
> 416e7f0c9d613b Janis Schoetterl-Glausch 2021-11-26 1059 len -= fragment_len;
> 416e7f0c9d613b Janis Schoetterl-Glausch 2021-11-26 1060 data += fragment_len;
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1061 ga = kvm_s390_logical_to_effective(vcpu, ga + fragment_len);
> 2293897805c2fe Heiko Carstens 2014-01-01 1062 }
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1063 if (rc > 0)
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 @1064 rc = trans_exc(vcpu, rc, ga, ar, mode, prot);
>
> Smatch is not using the cross function DB here so it assumes other
> positive values are possible. Also "prot" might not be used in the
> trans_exc() but smatch will still complain instead of checking for
> that.
Indeed, prot is only access by trans_exc in case of PGM_PROTECTION.
That also makes a default value questionable/confusing.
>
>
> e613d83454d7da Janis Schoetterl-Glausch 2022-02-11 1065 out_unlock:
> 8a242234b4bfed Heiko Carstens 2014-01-10 1066 if (need_ipte_lock)
> 8a242234b4bfed Heiko Carstens 2014-01-10 1067 ipte_unlock(vcpu);
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1068 if (nr_pages > ARRAY_SIZE(gpa_array))
> 7faa543df19bf6 Janis Schoetterl-Glausch 2021-11-26 1069 vfree(gpas);
> 2293897805c2fe Heiko Carstens 2014-01-01 1070 return rc;
> 2293897805c2fe Heiko Carstens 2014-01-01 1071 }
>
Powered by blists - more mailing lists