[PATCH BlueZ] btmon-logger: Fix stack corruption

[PATCH BlueZ] btmon-logger: Fix stack corruption

[Mariusz Kozłowski]

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];
 	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

RE: [BlueZ] btmon-logger: Fix stack corruption

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

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

Re: [PATCH BlueZ] btmon-logger: Fix stack corruption

[Luiz Augusto von Dentz]

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
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] btmon-logger: Fix stack corruption

[Mariusz Kozlowski]

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 
 >  
 > 

Re: [PATCH BlueZ] btmon-logger: Fix stack corruption

[Mariusz Kozlowski]

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