2024-01-21 10:04:34

by Mariusz Kozlowski

[permalink] [raw]
Subject: [PATCH BlueZ] btmon-logger: Fix stack corruption

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



2024-01-21 11:07:03

by bluez.test.bot

[permalink] [raw]
Subject: RE: [BlueZ] btmon-logger: Fix stack corruption

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

2024-01-22 19:09:06

by Luiz Augusto von Dentz

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

Hi Mariusz,

On Sun, Jan 21, 2024 at 5:04 AM Mariusz Kozłowski <[email protected]> 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

2024-01-23 08:13:25

by Mariusz Kozlowski

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

Hi Luiz,

> Hi Mariusz,
>
> On Sun, Jan 21, 2024 at 5:04 AM Mariusz Kozłowski [email protected]> 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
>
>

2024-02-12 08:37:33

by Mariusz Kozlowski

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

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 [email protected]> 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