Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-04 Thread Masami Hiramatsu
On Fri, 4 Dec 2020 12:06:44 +0100 Borislav Petkov wrote: > On Fri, Dec 04, 2020 at 09:56:53AM +0900, Masami Hiramatsu wrote: > > Hmm, there is a difference between Intel SDM and AMD APM. > > > > Intel SDM vol.2 > > > > 2.1.1 Instruction Prefixes > > Instruction prefixes are divided into four

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-04 Thread Borislav Petkov
On Fri, Dec 04, 2020 at 09:56:53AM +0900, Masami Hiramatsu wrote: > Hmm, there is a difference between Intel SDM and AMD APM. > > Intel SDM vol.2 > > 2.1.1 Instruction Prefixes > Instruction prefixes are divided into four groups, each with a set of > allowable prefix codes. For each

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Masami Hiramatsu
On Fri, 4 Dec 2020 09:56:53 +0900 Masami Hiramatsu wrote: > On Thu, 3 Dec 2020 12:49:46 -0600 > Tom Lendacky wrote: > > > On 12/3/20 12:17 PM, Borislav Petkov wrote: > > > On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote: > > >> Since that struct is used in multiple places, I think

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Masami Hiramatsu
On Thu, 3 Dec 2020 12:49:46 -0600 Tom Lendacky wrote: > On 12/3/20 12:17 PM, Borislav Petkov wrote: > > On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote: > >> Since that struct is used in multiple places, I think basing it on the > >> array > >> size is the best way to go. The main

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Masami Hiramatsu
On Thu, 3 Dec 2020 13:37:57 +0100 Borislav Petkov wrote: > On Thu, Dec 03, 2020 at 01:50:37PM +0900, Masami Hiramatsu wrote: > > Since the insn.prefixes.nbytes can be bigger than the size of > > insn.prefixes.bytes[] when a same prefix is repeated, we have to > > check whether the

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Masami Hiramatsu
On Thu, 3 Dec 2020 10:45:48 -0600 Tom Lendacky wrote: > On 12/3/20 6:48 AM, Borislav Petkov wrote: > > So it ended up like this: > > > > --- > > From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001 > > From: Masami Hiramatsu > > Date: Thu, 3 Dec 2020 13:50:37 +0900 > >

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Tom Lendacky
On 12/3/20 12:17 PM, Borislav Petkov wrote: On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote: Since that struct is used in multiple places, I think basing it on the array size is the best way to go. The main point of the check is just to be sure you don't read outside of the array.

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
On Thu, Dec 03, 2020 at 12:10:10PM -0600, Tom Lendacky wrote: > Since that struct is used in multiple places, I think basing it on the array > size is the best way to go. The main point of the check is just to be sure > you don't read outside of the array. Well, what happens if someone increases

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Tom Lendacky
On 12/3/20 11:01 AM, Borislav Petkov wrote: On Thu, Dec 03, 2020 at 05:54:20PM +0100, Borislav Petkov wrote: On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote: Since this is based on the array size, can idx < NUM_LEGACY_PREFIXES be replaced with: idx <

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
On Thu, Dec 03, 2020 at 05:54:20PM +0100, Borislav Petkov wrote: > On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote: > > Since this is based on the array size, can > > > > idx < NUM_LEGACY_PREFIXES > > > > be replaced with: > > > > idx < ARRAY_SIZE(insn->prefixes.bytes) > >

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
On Thu, Dec 03, 2020 at 10:45:48AM -0600, Tom Lendacky wrote: > Since this is based on the array size, can > > idx < NUM_LEGACY_PREFIXES > > be replaced with: > > idx < ARRAY_SIZE(insn->prefixes.bytes) Actually, this needs another change: struct insn_field { union {

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Tom Lendacky
On 12/3/20 6:48 AM, Borislav Petkov wrote: So it ended up like this: --- From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001 From: Masami Hiramatsu Date: Thu, 3 Dec 2020 13:50:37 +0900 Subject: [PATCH] x86/uprobes: Do not use prefixes.nbytes when looping over

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
So it ended up like this: --- >From 5014e4e902778d63ce392f864b3654baa4b72384 Mon Sep 17 00:00:00 2001 From: Masami Hiramatsu Date: Thu, 3 Dec 2020 13:50:37 +0900 Subject: [PATCH] x86/uprobes: Do not use prefixes.nbytes when looping over prefixes.bytes Since insn.prefixes.nbytes can be bigger

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
On Thu, Dec 03, 2020 at 01:37:57PM +0100, Borislav Petkov wrote: > Btw, looking at the struct insn definition, that prefixes member should > have a comment above it that those are the legacy prefixes which can be > <= 4. But that's minor. And that naked 4 is poking my eye too - It'd be better if

Re: [PATCH v2 1/3] x86/uprobes: Fix not using prefixes.nbytes for loop over prefixes.bytes

2020-12-03 Thread Borislav Petkov
On Thu, Dec 03, 2020 at 01:50:37PM +0900, Masami Hiramatsu wrote: > Since the insn.prefixes.nbytes can be bigger than the size of > insn.prefixes.bytes[] when a same prefix is repeated, we have to > check whether the insn.prefixes.bytes[i] != 0 and i < 4 instead > of insn.prefixes.nbytes. > This