| Message ID | 20240121100328.1200839-1-mk@lab.zgora.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [BlueZ] btmon-logger: Fix stack corruption | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=818351 ---Test result--- Test Summary: CheckPatch PASS 0.44 seconds GitLint PASS 0.31 seconds BuildEll PASS 23.70 seconds BluezMake PASS 693.93 seconds MakeCheck PASS 12.21 seconds MakeDistcheck PASS 157.89 seconds CheckValgrind PASS 220.46 seconds CheckSmatch PASS 324.72 seconds bluezmakeextell PASS 105.36 seconds IncrementalBuild PASS 649.47 seconds ScanBuild PASS 937.56 seconds --- Regards, Linux Bluetooth
Hi Mariusz, On Sun, Jan 21, 2024 at 5:04 AM Mariusz Kozłowski <mk@lab.zgora.pl> wrote: > > Version 3 capability masks are 64 bits in size. > --- > tools/btmon-logger.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/tools/btmon-logger.c b/tools/btmon-logger.c > index a770ad575..1f6db3751 100644 > --- a/tools/btmon-logger.c > +++ b/tools/btmon-logger.c > @@ -161,14 +161,14 @@ extern int capset(struct __user_cap_header_struct *header, > static void drop_capabilities(void) > { > struct __user_cap_header_struct header; > - struct __user_cap_data_struct cap; > + struct __user_cap_data_struct cap[_LINUX_CAPABILITY_U32S_3]; Ok, but this doesn't change the field, it makes it an array, or are you talking about the following note: Note that 64-bit capabilities use datap[0] and datap[1], whereas 32-bit capabilities use only datap[0]. In that case Ive just pointed out to this note to explain why this is needed. > unsigned int mask; > int err; > > header.version = _LINUX_CAPABILITY_VERSION_3; > header.pid = 0; > > - err = capget(&header, &cap); > + err = capget(&header, cap); > if (err) { > perror("Unable to get current capabilities"); > return; > @@ -177,11 +177,11 @@ static void drop_capabilities(void) > /* not needed anymore since monitor socket is already open */ > mask = ~CAP_TO_MASK(CAP_NET_RAW); > > - cap.effective &= mask; > - cap.permitted &= mask; > - cap.inheritable &= mask; > + cap[0].effective &= mask; > + cap[0].permitted &= mask; > + cap[0].inheritable &= mask; > > - err = capset(&header, &cap); > + err = capset(&header, cap); > if (err) > perror("Failed to set capabilities"); > } > -- > 2.34.1 > >
Hi Luiz, > Hi Mariusz, > > On Sun, Jan 21, 2024 at 5:04 AM Mariusz Kozłowski mk@lab.zgora.pl> wrote: > > > > Version 3 capability masks are 64 bits in size. > > --- > > tools/btmon-logger.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/tools/btmon-logger.c b/tools/btmon-logger.c > > index a770ad575..1f6db3751 100644 > > --- a/tools/btmon-logger.c > > +++ b/tools/btmon-logger.c > > @@ -161,14 +161,14 @@ extern int capset(struct __user_cap_header_struct *header, > > static void drop_capabilities(void) > > { > > struct __user_cap_header_struct header; > > - struct __user_cap_data_struct cap; > > + struct __user_cap_data_struct cap[_LINUX_CAPABILITY_U32S_3]; > > Ok, but this doesn't change the field, it makes it an array, or are > you talking about the following note: > > Note that 64-bit capabilities use datap[0] and datap[1], whereas > 32-bit capabilities use only datap[0]. > > In that case Ive just pointed out to this note to explain why this is needed. For version 3 caps (64 bit masks) a single struct __user_cap_data_struct is not big enough and capget() writes past the end of cap structure on the stack. To accomodate version 3 cap masks the cap structure needs to be 2x bigger. > > unsigned int mask; > > int err; > > > > header.version = _LINUX_CAPABILITY_VERSION_3; > > header.pid = 0; > > > > - err = capget(&header, &cap); > > + err = capget(&header, cap); > > if (err) { > > perror("Unable to get current capabilities"); > > return; > > @@ -177,11 +177,11 @@ static void drop_capabilities(void) > > /* not needed anymore since monitor socket is already open */ > > mask = ~CAP_TO_MASK(CAP_NET_RAW); > > > > - cap.effective &= mask; > > - cap.permitted &= mask; > > - cap.inheritable &= mask; > > + cap[0].effective &= mask; > > + cap[0].permitted &= mask; > > + cap[0].inheritable &= mask; > > > > - err = capset(&header, &cap); > > + err = capset(&header, cap); > > if (err) > > perror("Failed to set capabilities"); > > } > > -- > > 2.34.1 > > > > > > > -- > Luiz Augusto von Dentz > >
Hi Luiz, ---- On Tue, 23 Jan 2024 09:12:50 +0100 Mariusz Kozlowski wrote --- > Hi Luiz, > > > Hi Mariusz, > > > > On Sun, Jan 21, 2024 at 5:04 AM Mariusz Kozłowski mk@lab.zgora.pl> wrote: > > > > > > Version 3 capability masks are 64 bits in size. > > > --- > > > tools/btmon-logger.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/tools/btmon-logger.c b/tools/btmon-logger.c > > > index a770ad575..1f6db3751 100644 > > > --- a/tools/btmon-logger.c > > > +++ b/tools/btmon-logger.c > > > @@ -161,14 +161,14 @@ extern int capset(struct __user_cap_header_struct *header, > > > static void drop_capabilities(void) > > > { > > > struct __user_cap_header_struct header; > > > - struct __user_cap_data_struct cap; > > > + struct __user_cap_data_struct cap[_LINUX_CAPABILITY_U32S_3]; > > > > Ok, but this doesn't change the field, it makes it an array, or are > > you talking about the following note: > > > > Note that 64-bit capabilities use datap[0] and datap[1], whereas > > 32-bit capabilities use only datap[0]. > > > > In that case Ive just pointed out to this note to explain why this is needed. > > For version 3 caps (64 bit masks) a single struct __user_cap_data_struct is not > big enough and capget() writes past the end of cap structure on the stack. To > accomodate version 3 cap masks the cap structure needs to be 2x bigger. What is the status of this patch? I don't see it either accepted or rejected. > > > unsigned int mask; > > > int err; > > > > > > header.version = _LINUX_CAPABILITY_VERSION_3; > > > header.pid = 0; > > > > > > - err = capget(&header, &cap); > > > + err = capget(&header, cap); > > > if (err) { > > > perror("Unable to get current capabilities"); > > > return; > > > @@ -177,11 +177,11 @@ static void drop_capabilities(void) > > > /* not needed anymore since monitor socket is already open */ > > > mask = ~CAP_TO_MASK(CAP_NET_RAW); > > > > > > - cap.effective &= mask; > > > - cap.permitted &= mask; > > > - cap.inheritable &= mask; > > > + cap[0].effective &= mask; > > > + cap[0].permitted &= mask; > > > + cap[0].inheritable &= mask; > > > > > > - err = capset(&header, &cap); > > > + err = capset(&header, cap); > > > if (err) > > > perror("Failed to set capabilities"); > > > } Thanks, Mariusz
diff --git a/tools/btmon-logger.c b/tools/btmon-logger.c index a770ad575..1f6db3751 100644 --- a/tools/btmon-logger.c +++ b/tools/btmon-logger.c @@ -161,14 +161,14 @@ extern int capset(struct __user_cap_header_struct *header, static void drop_capabilities(void) { struct __user_cap_header_struct header; - struct __user_cap_data_struct cap; + struct __user_cap_data_struct cap[_LINUX_CAPABILITY_U32S_3]; unsigned int mask; int err; header.version = _LINUX_CAPABILITY_VERSION_3; header.pid = 0; - err = capget(&header, &cap); + err = capget(&header, cap); if (err) { perror("Unable to get current capabilities"); return; @@ -177,11 +177,11 @@ static void drop_capabilities(void) /* not needed anymore since monitor socket is already open */ mask = ~CAP_TO_MASK(CAP_NET_RAW); - cap.effective &= mask; - cap.permitted &= mask; - cap.inheritable &= mask; + cap[0].effective &= mask; + cap[0].permitted &= mask; + cap[0].inheritable &= mask; - err = capset(&header, &cap); + err = capset(&header, cap); if (err) perror("Failed to set capabilities"); }