The byteOffset operation doesn't load, and just returns an int. It's fine to not auth in this scenario.
Created attachment 405534 [details] patch
Comment on attachment 405534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review > Source/JavaScriptCore/ChangeLog:15 > + Since byteOffset does no loads/stores, it suffices to just strip the PAC > + bits before doing the subtraction. This eliminates any such attacks like > + the above because the PAC bits are ignored. Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits?
(In reply to Darin Adler from comment #2) > Comment on attachment 405534 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405534&action=review > > > Source/JavaScriptCore/ChangeLog:15 > > + Since byteOffset does no loads/stores, it suffices to just strip the PAC > > + bits before doing the subtraction. This eliminates any such attacks like > > + the above because the PAC bits are ignored. > > Just curious: Why is stripping needed? Won’t both pointers have the same PAC > bits? One of the pointers (the base) has no PAC bits.
Comment on attachment 405534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review >>> Source/JavaScriptCore/ChangeLog:15 >>> + the above because the PAC bits are ignored. >> >> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? > > One of the pointers (the base) has no PAC bits. This isn't true. Ignore me.
Comment on attachment 405534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review >>>> Source/JavaScriptCore/ChangeLog:15 >>>> + the above because the PAC bits are ignored. >>> >>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >> >> One of the pointers (the base) has no PAC bits. > > This isn't true. Ignore me. The real reason is this: The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do.
Comment on attachment 405534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review I can almost review, but not quite. The code looks right to me, but I don’t know enough about conventions in this code to be a helpful reviewer. >>>>> Source/JavaScriptCore/ChangeLog:15 >>>>> + the above because the PAC bits are ignored. >>>> >>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >>> >>> One of the pointers (the base) has no PAC bits. >> >> This isn't true. Ignore me. > > The real reason is this: > > The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. In some cases like this I have seen people do arithmetic and then strip the result, to do half a much stripping. But I have no idea if this is relevant, acceptable, and valuable enough to be worthwhile here.
(In reply to Darin Adler from comment #6) > Comment on attachment 405534 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405534&action=review > > I can almost review, but not quite. The code looks right to me, but I don’t > know enough about conventions in this code to be a helpful reviewer. > > >>>>> Source/JavaScriptCore/ChangeLog:15 > >>>>> + the above because the PAC bits are ignored. > >>>> > >>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? > >>> > >>> One of the pointers (the base) has no PAC bits. > >> > >> This isn't true. Ignore me. > > > > The real reason is this: > > > > The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. > > In some cases like this I have seen people do arithmetic and then strip the > result, to do half a much stripping. But I have no idea if this is relevant, > acceptable, and valuable enough to be worthwhile here. Let me look it up in some CPU docs. My assumption in writing this code is that stripping is 1-cycle (since it's essentially a masking operation). Even if it's not, to do the arithmetic, I think we'd need to do an extra load to get to the number that we need to do arithmetic on.
Comment on attachment 405534 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=405534&action=review r=me. >>>>>>> Source/JavaScriptCore/ChangeLog:15 >>>>>>> + the above because the PAC bits are ignored. >>>>>> >>>>>> Just curious: Why is stripping needed? Won’t both pointers have the same PAC bits? >>>>> >>>>> One of the pointers (the base) has no PAC bits. >>>> >>>> This isn't true. Ignore me. >>> >>> The real reason is this: >>> >>> The base may be a vector of length M. Signed using M. The view into that vector has length N, signed using N. N has to be <= M. Therefore, they might have the same bits, when M == N, but there is no guarantee they do. >> >> In some cases like this I have seen people do arithmetic and then strip the result, to do half a much stripping. But I have no idea if this is relevant, acceptable, and valuable enough to be worthwhile here. > > Let me look it up in some CPU docs. My assumption in writing this code is that stripping is 1-cycle (since it's essentially a masking operation). Even if it's not, to do the arithmetic, I think we'd need to do an extra load to get to the number that we need to do arithmetic on. From conversations with CPU folks, my understanding is that stripping is "free". And FWIW, at least on Apple Silicon ™️, it's also "free" to do an auth as long as the result of that auth doesn't flow (either directly or via a load/store) into a branch.
Committed r265097: <https://trac.webkit.org/changeset/265097> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405534 [details].
<rdar://problem/66348653>
Re-opened since this is blocked by bug 215065