On 20.09.2019 11:24, Jan Beulich wrote: > On 20.09.2019 10:06, Alexandru Stefan ISAILA wrote: >> On 19.09.2019 16:59, Jan Beulich wrote: >>> Furthermore while you now restrict the check to linear address >>> based accesses, other than the description says (or at least >>> implies) you do not restrict it to read and exec accesses. It's >>> not clear to me whether that's intentional, yet it affects which >>> hvm_copy_*_linear() callers need auditing. >> >> The pfec var is checked in hvm_monitor_check_p2m(). If you think it is >> necessary I can add one more check here for (pfec & (PFEC_insn_fetch | >> PFEC_write_access)). > > hvm_monitor_check_p2m() gets called from two places, so a check > there won't help (afaict). The question whether to put an > additional check here depends on whether, as the description > says, you really only want to handle read/exec accesses here, > or whether you also want to cover write ones (in which case the > description should be adjusted so that it's not misleading).
Indeed covering write access here as well as in hvmemul_map_linear_addr() is needed. I will adjust the comment. > >>> Finally, what about ->arch.vm_event->send_event remaining set >>> after hvm_emulate_one_vm_event(), because hvm_monitor_check_p2m() >>> (the only place where the flag would get cleared) was never hit >>> in the process? >> Thanks for pointing this out, indeed it's a problem here. A solution can >> be to move send_event = false; after hvm_emulate_one_vm_event() is >> finished. And state in the comment that the user is in charge of >> enabling and disabling the flag. >> Or just have it in both places. > > For this aspect alone I think you want it in both places, but ... > >>> And what about an instruction accessing two (or >>> more) distinct addresses? The flag would be clear after the first >>> one was checked afaict. >> >> There is no problem here because emulation will stop after the first bad >> access so there is no need to continue. > > ... for this moving it may indeed be necessary. I have to admit > that I don't follow your reply here: The flag will also be clear > after the first good access (afaict), and hence further accesses > by the same insn won't make it into hvm_monitor_check_p2m() at all. > Ok I will move it from hvm_monitor_check_p2m() and adjust the comment accordingly. Thanks for the review, Alex _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
