* RE: [PATCH] ks8851: Cancel any pending IRQ work
From: Matt Renzelmann @ 2012-04-12 20:34 UTC (permalink / raw)
To: 'Stephen Boyd'; +Cc: davem, ben, netdev, Matt Renzelmann
In-Reply-To: <4F8738EB.2060806@codeaurora.org>
>
> Is this actually solving anything? Presumably cancel_work_sync() could
> run and then another spurious interrupt could come in after that
> function returns and we would have the same problem again. We should
> probably free the irq before unregistering the netdev so that
> ks8851_net_stop() would run after the interrupt is no longer registered,
> and the flush_work() in there would finish the last work. But then we
> have a problem where we're enabling the irq in the irq_work callback
> after the irq has been freed. Ugh.
>
> I also see a potential deadlock in ks8851_net_stop(). ks8851_net_stop()
> holds the ks->lock while calling flush_work() which could deadlock if an
> interrupt comes and schedules an irq_work between the time
> ks8851_net_stop() grabs the mutex and calls flush_work().
>
I agree on all counts -- the patch is buggy, though it does at least "shrink"
the window of vulnerability. Frankly, I don't believe I'm qualified to write an
appropriate patch for this driver, at least without spending considerably more
time on it.
FWIW, I found this problem with a new driver-testing tool we've developed called
SymDrive, and my goal is primarily to determine if the bug is real or not. The
tool is imperfect and we are trying to validate its operation.
That said, if there is an issue here, and we can come up with an appropriate
fix, then I'd be happy to write a patch for it.
^ permalink raw reply
* Re: [PATCH] fixed coding style issue
From: David Miller @ 2012-04-12 20:36 UTC (permalink / raw)
To: ahiliation; +Cc: netdev
In-Reply-To: <20120408160742.GA4613@debian.Jeff>
From: Jeffrin Jose <ahiliation@yahoo.co.in>
Date: Sun, 8 Apr 2012 21:37:42 +0530
> i have worked on some coding style issues
> on net/core/utils.c
>
> please see attachment.
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 20:40 UTC (permalink / raw)
To: mjr; +Cc: davem, sboyd, ben, netdev
In-Reply-To: <1334261204-8554-1-git-send-email-mjr@cs.wisc.edu>
On Thu, 12 Apr 2012 15:06:44 -0500
mjr@cs.wisc.edu wrote:
> From: Matt Renzelmann <mjr@cs.wisc.edu>
>
> All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> the driver's lock mutex. A spurious interrupt may otherwise cause a
> crash.
>
> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> ---
>
> Thank you, Mr. Leitner, for providing feedback. I agree with your
> changes and have updated the patch to reflect them. I apologize for
> missing the driver name in the title -- I've updated the patch with
> that information as well. Please let me know if there is anything
> else I should fix/change.
Signed-off-by: Flavio Leitner <fbl@redhat.com>
^ permalink raw reply
* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Greear @ 2012-04-12 20:46 UTC (permalink / raw)
To: Ben Hutchings
Cc: Florian Fainelli, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1334259053.2497.18.camel-/LGg1Z1CJKReKY3V0RtoKmatzQS1i7+A3tAM5lWOD0I@public.gmane.org>
On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>> Hi,
>>>
>>> Le 04/12/12 18:32, greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org a écrit :
>>>> From: Ben Greear<greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
>>>>
>>>> The signal and noise are forced to be positive since ethtool
>>>> deals in unsigned 64-bit values and this number should be human
>>>> readable. This gives easy access to some of the data formerly
>>>> exposed in the deprecated /proc/net/wireless file.
>>>
>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>> understand it. Does not ethtool know about 32-bits signed integers?
>>
>> Ethtool stats only supports u64. I think it's easy enough for
>> humans or programs to add the negative sign. Can signal or noise
>> ever be> 0? If so, that could actually break something that depends
>> on flipping the value to negative....
>
> So far as I can see, the ethtool stats were expected to be counters,
> which obviously cannot become negative (or fractional). Maybe it's time
> to define another command and string-set to cover other types of status
> information. The ethtool utility could ask for those typed statistics
> as well, so 'ethtool -S' would get all of them.
One nice thing about ethtool stats API is that it is backwards and forwards
compatible for a long while. Also, adding a new API means a second call
into the kernel (most likely) for the other set of strings, which effectively
doubles the cost of getting stats (and allows the stats to be updated
out-of-sync with each other more easily).
I wonder if instead we could add a convention where we add a short
prefix (or suffix) to a string to denote it's type (and cast the value
into u64). So, an old ethtool might see:
foo:s32: 4294967295
while a new one understand that the s32: prefix is special, do
some casting, and could show:
foo: -1
Both are still at least somewhat human readable, and probably wouldn't
confuse anyone that is parsing the output of existing ethtool output.
Thanks,
Ben
--
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Matt Renzelmann @ 2012-04-12 20:56 UTC (permalink / raw)
To: 'Stephen Boyd', mjr; +Cc: fbl, davem, ben, netdev
In-Reply-To: <4F873C58.3000104@codeaurora.org>
> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?
>
The reason I didn't go that way was the request_irq call is not made until near
the end of probe. I believe only the ks8851_rdreg16 call in the netdev_info
statement actually needs the mutex protection. That said, the approach you
posted looks reasonable as well and may be clearer. I'm unsure as to which way
is best.
Thanks everyone for your help,
Matt
^ permalink raw reply
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 20:59 UTC (permalink / raw)
To: Stephen Boyd; +Cc: mjr, davem, ben, netdev
In-Reply-To: <4F873C58.3000104@codeaurora.org>
On Thu, 12 Apr 2012 13:34:32 -0700
Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > From: Matt Renzelmann <mjr@cs.wisc.edu>
> >
> > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > the driver's lock mutex. A spurious interrupt may otherwise cause a
> > crash.
> >
> > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > ---
> >
> > Thank you, Mr. Leitner, for providing feedback. I agree with your
> > changes and have updated the patch to reflect them. I apologize for
> > missing the driver name in the title -- I've updated the patch with
> > that information as well. Please let me know if there is anything
> > else I should fix/change.
> >
> > drivers/net/ethernet/micrel/ks8851.c | 8 ++++++--
> > 1 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > index c722aa6..20237dc 100644
> > --- a/drivers/net/ethernet/micrel/ks8851.c
> > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > {
> > struct net_device *ndev;
> > struct ks8851_net *ks;
> > + int result;
> > int ret;
> >
> > ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > goto err_netdev;
> > }
> >
> > + mutex_lock(&ks->lock);
> > + result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > + mutex_unlock(&ks->lock);
> > +
> > netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > - CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > - ndev->dev_addr, ndev->irq,
> > + result, ndev->dev_addr, ndev->irq,
> > ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> >
> > return 0;
>
> This register is already read in the probe function and the lock is not
> held there so you seem to have missed a couple. I would guess it doesn't
> really matter tha we don't grab the lock though because the device isn't
> actively sending/receiving packets. How about this instead?
I believe that's because the IRQ isn't reserved yet, so there is no problem.
> diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> index c722aa60..6f21fcd 100644
> --- a/drivers/net/ethernet/micrel/ks8851.c
> +++ b/drivers/net/ethernet/micrel/ks8851.c
> @@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> struct net_device *ndev;
> struct ks8851_net *ks;
> int ret;
> + unsigned cider;
>
> ndev = alloc_etherdev(sizeof(struct ks8851_net));
> if (!ndev)
> @@ -1484,8 +1485,9 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> ks8851_soft_reset(ks, GRR_GSR);
>
> /* simple check for a valid chip being connected to the bus */
> -
> - if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
> + mutex_lock(&ks->lock);
> + cider = ks8851_rdreg16(ks, KS_CIDER);
> + if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
> dev_err(&spi->dev, "failed to read device ID\n");
If it fails here, the mutex is left locked.
> ret = -ENODEV;
> goto err_id;
> @@ -1493,6 +1495,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
>
> /* cache the contents of the CCR register for EEPROM, etc. */
> ks->rc_ccr = ks8851_rdreg16(ks, KS_CCR);
> + mutex_unlock(&ks->lock);
>
> if (ks->rc_ccr & CCR_EEPROM)
> ks->eeprom_size = 128;
> @@ -1516,8 +1519,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> }
>
> netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> - CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> - ndev->dev_addr, ndev->irq,
> + CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
> ks->rc_ccr & CCR_EEPROM ? "has" : "no");
>
> return 0;
I don't know the hw specs, but assuming that revision doesn't change between
those reads or that reading it doesn't do anything else... the above looks
better, indeed.
fbl
^ permalink raw reply
* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Hutchings @ 2012-04-12 21:05 UTC (permalink / raw)
To: Ben Greear; +Cc: Florian Fainelli, linux-wireless, netdev
In-Reply-To: <4F873F10.6010207@candelatech.com>
On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
> > On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
> >> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
> >>> Hi,
> >>>
> >>> Le 04/12/12 18:32, greearb@candelatech.com a écrit :
> >>>> From: Ben Greear<greearb@candelatech.com>
> >>>>
> >>>> The signal and noise are forced to be positive since ethtool
> >>>> deals in unsigned 64-bit values and this number should be human
> >>>> readable. This gives easy access to some of the data formerly
> >>>> exposed in the deprecated /proc/net/wireless file.
> >>>
> >>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
> >>> understand it. Does not ethtool know about 32-bits signed integers?
> >>
> >> Ethtool stats only supports u64. I think it's easy enough for
> >> humans or programs to add the negative sign. Can signal or noise
> >> ever be> 0? If so, that could actually break something that depends
> >> on flipping the value to negative....
> >
> > So far as I can see, the ethtool stats were expected to be counters,
> > which obviously cannot become negative (or fractional). Maybe it's time
> > to define another command and string-set to cover other types of status
> > information. The ethtool utility could ask for those typed statistics
> > as well, so 'ethtool -S' would get all of them.
>
> One nice thing about ethtool stats API is that it is backwards and forwards
> compatible for a long while.
Agreed.
> Also, adding a new API means a second call
> into the kernel (most likely) for the other set of strings, which effectively
> doubles the cost of getting stats (and allows the stats to be updated
> out-of-sync with each other more easily).
It's generally not possible to take an atomic snapshot of all statistics
for a NIC, so that shouldn't be a consideration.
> I wonder if instead we could add a convention where we add a short
> prefix (or suffix) to a string to denote it's type (and cast the value
> into u64). So, an old ethtool might see:
>
> foo:s32: 4294967295
Actually it would be:
foo:s32: 18446744073709551615
> while a new one understand that the s32: prefix is special, do
> some casting, and could show:
> foo: -1
>
> Both are still at least somewhat human readable,
I don't think many humans can mentally substract 2^64.
> and probably wouldn't confuse anyone that is parsing the output of
> existing ethtool output.
I think you have this backwards: printing numbers in two different ways
(old and new versions of ethtool) is likely to confuse scripts that are
parsing and doing calculations with these numbers. While I try to avoid
gratuitous changes in output formatting, scripts should use the ethtool
API if they really want interface stability. It's not difficult (there
are at least Python and Perl bindings available) and it's a lot more
efficient.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] ks8851: Fix missing mutex_lock/unlock
From: Flavio Leitner @ 2012-04-12 21:19 UTC (permalink / raw)
To: Flavio Leitner; +Cc: Stephen Boyd, mjr, davem, ben, netdev
In-Reply-To: <20120412175945.1bbdd9bb@asterix.rh>
On Thu, 12 Apr 2012 17:59:45 -0300
Flavio Leitner <fbl@redhat.com> wrote:
> On Thu, 12 Apr 2012 13:34:32 -0700
> Stephen Boyd <sboyd@codeaurora.org> wrote:
>
> > On 04/12/12 13:06, mjr@cs.wisc.edu wrote:
> > > From: Matt Renzelmann <mjr@cs.wisc.edu>
> > >
> > > All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
> > > the driver's lock mutex. A spurious interrupt may otherwise cause a
> > > crash.
> > >
> > > Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> > > ---
> > >
> > > Thank you, Mr. Leitner, for providing feedback. I agree with your
> > > changes and have updated the patch to reflect them. I apologize for
> > > missing the driver name in the title -- I've updated the patch with
> > > that information as well. Please let me know if there is anything
> > > else I should fix/change.
> > >
> > > drivers/net/ethernet/micrel/ks8851.c | 8 ++++++--
> > > 1 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
> > > index c722aa6..20237dc 100644
> > > --- a/drivers/net/ethernet/micrel/ks8851.c
> > > +++ b/drivers/net/ethernet/micrel/ks8851.c
> > > @@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > > {
> > > struct net_device *ndev;
> > > struct ks8851_net *ks;
> > > + int result;
> > > int ret;
> > >
> > > ndev = alloc_etherdev(sizeof(struct ks8851_net));
> > > @@ -1515,9 +1516,12 @@ static int __devinit ks8851_probe(struct spi_device *spi)
> > > goto err_netdev;
> > > }
> > >
> > > + mutex_lock(&ks->lock);
> > > + result = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
> > > + mutex_unlock(&ks->lock);
> > > +
> > > netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
> > > - CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
> > > - ndev->dev_addr, ndev->irq,
> > > + result, ndev->dev_addr, ndev->irq,
> > > ks->rc_ccr & CCR_EEPROM ? "has" : "no");
> > >
> > > return 0;
> >
> > This register is already read in the probe function and the lock is not
> > held there so you seem to have missed a couple. I would guess it doesn't
> > really matter tha we don't grab the lock though because the device isn't
> > actively sending/receiving packets. How about this instead?
>
> I believe that's because the IRQ isn't reserved yet, so there is no problem.
So, what about this instead:
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..7137f47 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1417,6 +1417,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
{
struct net_device *ndev;
struct ks8851_net *ks;
+ unsigned int cider;
int ret;
ndev = alloc_etherdev(sizeof(struct ks8851_net));
@@ -1485,7 +1486,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
/* simple check for a valid chip being connected to the bus */
- if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+ cider = ks8851_rdreg16(ks, KS_CIDER);
+ if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
dev_err(&spi->dev, "failed to read device ID\n");
ret = -ENODEV;
goto err_id;
@@ -1516,7 +1518,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
}
netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
- CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
+ CIDER_REV_GET(cider),
ndev->dev_addr, ndev->irq,
ks->rc_ccr & CCR_EEPROM ? "has" : "no");
fbl
^ permalink raw reply related
* Re: [PATCH 4/5] mac80211: Add more ethtools stats: survey, rates, etc
From: Ben Greear @ 2012-04-12 21:21 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Florian Fainelli, linux-wireless, netdev
In-Reply-To: <1334264733.2497.35.camel@bwh-desktop.uk.solarflarecom.com>
On 04/12/2012 02:05 PM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 13:46 -0700, Ben Greear wrote:
>> On 04/12/2012 12:30 PM, Ben Hutchings wrote:
>>> On Thu, 2012-04-12 at 09:51 -0700, Ben Greear wrote:
>>>> On 04/12/2012 09:42 AM, Florian Fainelli wrote:
>>>>> Hi,
>>>>>
>>>>> Le 04/12/12 18:32, greearb@candelatech.com a écrit :
>>>>>> From: Ben Greear<greearb@candelatech.com>
>>>>>>
>>>>>> The signal and noise are forced to be positive since ethtool
>>>>>> deals in unsigned 64-bit values and this number should be human
>>>>>> readable. This gives easy access to some of the data formerly
>>>>>> exposed in the deprecated /proc/net/wireless file.
>>>>>
>>>>> Uh, that's misleading, the signal and noise values are typically negative, so one needs to think about mentally adding a minus sign if he/she wants to
>>>>> understand it. Does not ethtool know about 32-bits signed integers?
>>>>
>>>> Ethtool stats only supports u64. I think it's easy enough for
>>>> humans or programs to add the negative sign. Can signal or noise
>>>> ever be> 0? If so, that could actually break something that depends
>>>> on flipping the value to negative....
>>>
>>> So far as I can see, the ethtool stats were expected to be counters,
>>> which obviously cannot become negative (or fractional). Maybe it's time
>>> to define another command and string-set to cover other types of status
>>> information. The ethtool utility could ask for those typed statistics
>>> as well, so 'ethtool -S' would get all of them.
>>
>> One nice thing about ethtool stats API is that it is backwards and forwards
>> compatible for a long while.
>
> Agreed.
> Actually it would be:
>
> foo:s32: 18446744073709551615
>
>> while a new one understand that the s32: prefix is special, do
>> some casting, and could show:
>> foo: -1
>>
>> Both are still at least somewhat human readable,
>
> I don't think many humans can mentally substract 2^64.
Well, if we add a new API, then anyone on older ethtool
won't see it at all, which is even more useless than a
large ugly number.
Those of us using ethtool API directly would have to add new
ioctl calls (and the performance is important to me, even
if atomicity isn't so important). That is more work than
adding some logic to parse suffixes on the strings I think.
>> and probably wouldn't confuse anyone that is parsing the output of
>> existing ethtool output.
>
> I think you have this backwards: printing numbers in two different ways
> (old and new versions of ethtool) is likely to confuse scripts that are
> parsing and doing calculations with these numbers. While I try to avoid
> gratuitous changes in output formatting, scripts should use the ethtool
> API if they really want interface stability. It's not difficult (there
> are at least Python and Perl bindings available) and it's a lot more
> efficient.
If the new ethtool -S is going to nicely present things (ie, show "foo: -1"),
then the negative numbers are there anyway, so maybe the compatibility issue
for anyone parsing the output of 'ethtool -S' is moot. Anyone parsing the
binary API sees no changes, but *could* update code to look at the suffix
if they cared.
That said, I *would* like a new 'ethool get-stats' API that took a 'verbose'
argument so that we could return more or less verbose results (dependent on the
driver to determine what that means). That way, we could probe easy-to-obtain
information quickly and often, and if there is something more expensive to obtain,
that could be probed less often. If this idea is worth pursuing, then perhaps
it could also include a new binary API that includes a type identifier.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: linux-next: Tree for Apr 12
From: Andrew Morton @ 2012-04-12 21:24 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, LKML, netdev
In-Reply-To: <20120412145931.9ec6f2b51655071795d60d1d@canb.auug.org.au>
On Thu, 12 Apr 2012 14:59:31 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> I have created today's linux-next tree at
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
This isn't working for me. Some time between April 3 and April 12
someone merged something into the non-mm part of linux-next which broke
ssh.
I boot the box and everything seems to come up OK, but attemtps to ssh
into the machine fail with
X11 forwarding request failed on channel 0
Last login: Thu Apr 12 13:04:35 2012 from akpm.corp.google.com
Connection to akpm2 closed.
I took a peek in the `strace ssh' output.
Good:
17815 write(5, "Last login: Thu Apr 12 13:27:23 "..., 65) = 65
17815 select(7, [3 4], [], NULL, {120, 0}) = 1 (in [3], left {119, 770798})
17815 read(3, "\21O\200\366Mv\343\222\332\251\2403L\376Y18\2047\336\244\226p-+X\2%\2119\314\255"..., 8192) = 80
17815 select(7, [3 4], [5], NULL, {120, 0}) = 1 (out [5], left {119, 999987})
17815 write(5, "\r\33[m\17\33[27m\33[24m\33[Jakpm2:/home/ak"..., 39) = 39
17815 select(7, [3 4], [], NULL, {120, 0}) = 1 (in [4], left {118, 801111})
17815 read(4, "\4", 16384) = 1
17815 select(7, [3 4], [3], NULL, {120, 0}) = 1 (out [3], left {119, 999991})
17815 write(3, "\235J\5\340\234\21\266\207\26e\362\327\2\332\1\267\272\200\364\267?/\320L\341\35\350{+M:\222"..., 48) = 48
Bad:
9305 write(5, "Last login: Thu Apr 12 13:02:54 "..., 65) = 65
9305 select(7, [3 4], [], NULL, {120, 0}) = 1 (in [3], left {119, 945541})
9305 read(3, "f\357\250~\260i\2259\320\3258\262)O\364;_\251\360-\314\31\374]\326\300\356\364\370S\3105"..., 8192) = 128
9305 close(5) = 0
9305 close(4) = 0
That read() is returning a lot more data.
It appears that we've done something which breaks X forwarding. I
won't be able to look any further into this until Monday.
^ permalink raw reply
* Re: [PATCH] net/ipv6/exthdrs.c et al: Optional strict PadN option checking
From: Eldad Zack @ 2012-04-12 21:31 UTC (permalink / raw)
To: David Miller; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel
In-Reply-To: <20120412.160040.1086391259430365718.davem@davemloft.net>
On 12 April 2012 22:00, David Miller <davem@davemloft.net> wrote:
>> Added strict checking of PadN. PadN can be used to increase header
>> size and thus push the protocol header into the 2nd fragment.
>>
>> PadN is used to align the options within the Hop-by-Hop or
>> Destination Options header to 64-bit boundaries. The maximum valid
>> size is thus 7 bytes.
>> RFC 4942 recommends to actively check the "payload" itself and
>> ensure that it contains only zeroes.
> I think you should do away with the sysctl and always perform these
> checks.
>
> At the very leat, the optlen > 7 check should always be performed.
> And frankly the pad byte being zero check makes sense to do all the
> time as far as I can tell too.
That's the way I see it, as was my initial intent. Then I got
concerned with the possibility that a communication with
slightly-broken stack implementation (e.g., unsanitized buffers) would
fail without the user being able to control it at runtime.
Do you consider this a non-issue?
If not, please apply the (soon to be sent) patch.
Thanks,
Eldad
^ permalink raw reply
* [PATCH] net/ipv6/exthdrs.c: Strict PadN option checking
From: Eldad Zack @ 2012-04-12 21:31 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev, linux-kernel, Eldad Zack
Added strict checking of PadN, as PadN can be used to increase header
size and thus push the protocol header into the 2nd fragment.
PadN is used to align the options within the Hop-by-Hop or
Destination Options header to 64-bit boundaries. The maximum valid
size is thus 7 bytes.
RFC 4942 recommends to actively check the "payload" itself and
ensure that it contains only zeroes.
See also RFC 4942 section 2.1.9.5.
Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
---
net/ipv6/exthdrs.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index c486b8e..63f6e87 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -153,6 +153,7 @@ static int ip6_parse_tlv(struct tlvtype_proc *procs, struct sk_buff *skb)
while (len > 0) {
int optlen = nh[off + 1] + 2;
+ int i;
switch (nh[off]) {
case IPV6_TLV_PAD0:
@@ -160,6 +161,19 @@ static int ip6_parse_tlv(struct tlvtype_proc *procs, struct sk_buff *skb)
break;
case IPV6_TLV_PADN:
+ /* RFC 2460 states that the purpose of PadN is
+ to align the containing header to multiples
+ of 8. 7 is therefore the highest valid value.
+ See also RFC 4942, Section 2.1.9.5.*/
+ if (optlen > 7)
+ goto bad;
+ /* RFC 4942 recommends receiving hosts to
+ actively check PadN payload to contain
+ only zeroes. */
+ for (i = 2; i < optlen; i++) {
+ if (nh[off + i] != 0)
+ goto bad;
+ }
break;
default: /* Other TLV code so scan list */
--
1.7.9.5
^ permalink raw reply related
* [PATCH] ks8851: Fix missing mutex_lock/unlock
From: mjr @ 2012-04-12 21:28 UTC (permalink / raw)
To: fbl; +Cc: davem, sboyd, ben, netdev, Matt Renzelmann
From: Matt Renzelmann <mjr@cs.wisc.edu>
All calls to ks8851_rdreg* and ks8851_wrreg* should be protected with
the driver's lock mutex. A spurious interrupt may otherwise cause a
crash. Thanks to Stephen Boyd and Flavio Leitner for feedback.
Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
---
Here's the next revision. This is basically Flavio Leitner's latest
version without the mutex. Please let me know if I'm missing
anything and I can re-submit it.
drivers/net/ethernet/micrel/ks8851.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/micrel/ks8851.c b/drivers/net/ethernet/micrel/ks8851.c
index c722aa6..6528d66 100644
--- a/drivers/net/ethernet/micrel/ks8851.c
+++ b/drivers/net/ethernet/micrel/ks8851.c
@@ -1418,6 +1418,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
struct net_device *ndev;
struct ks8851_net *ks;
int ret;
+ unsigned cider;
ndev = alloc_etherdev(sizeof(struct ks8851_net));
if (!ndev)
@@ -1484,8 +1485,8 @@ static int __devinit ks8851_probe(struct spi_device *spi)
ks8851_soft_reset(ks, GRR_GSR);
/* simple check for a valid chip being connected to the bus */
-
- if ((ks8851_rdreg16(ks, KS_CIDER) & ~CIDER_REV_MASK) != CIDER_ID) {
+ cider = CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER));
+ if ((cider & ~CIDER_REV_MASK) != CIDER_ID) {
dev_err(&spi->dev, "failed to read device ID\n");
ret = -ENODEV;
goto err_id;
@@ -1516,8 +1517,7 @@ static int __devinit ks8851_probe(struct spi_device *spi)
}
netdev_info(ndev, "revision %d, MAC %pM, IRQ %d, %s EEPROM\n",
- CIDER_REV_GET(ks8851_rdreg16(ks, KS_CIDER)),
- ndev->dev_addr, ndev->irq,
+ CIDER_REV_GET(cider), ndev->dev_addr, ndev->irq,
ks->rc_ccr & CCR_EEPROM ? "has" : "no");
return 0;
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] net/ipv6/exthdrs.c et al: Optional strict PadN option checking
From: David Miller @ 2012-04-12 21:35 UTC (permalink / raw)
To: eldad; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel
In-Reply-To: <CABe5r8JJPuhq3r=JnfLA=_U46-SFmK+g3XqC3MaRdLxwVXx9Qw@mail.gmail.com>
From: Eldad Zack <eldad@fogrefinery.com>
Date: Thu, 12 Apr 2012 23:31:47 +0200
> That's the way I see it, as was my initial intent. Then I got
> concerned with the possibility that a communication with
> slightly-broken stack implementation (e.g., unsanitized buffers) would
> fail without the user being able to control it at runtime.
> Do you consider this a non-issue?
I think it's a non-issue.
^ permalink raw reply
* Re: [PATCH] net/ipv6/exthdrs.c: Strict PadN option checking
From: David Miller @ 2012-04-12 21:37 UTC (permalink / raw)
To: eldad; +Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel
In-Reply-To: <1334266320-12297-1-git-send-email-eldad@fogrefinery.com>
From: Eldad Zack <eldad@fogrefinery.com>
Date: Thu, 12 Apr 2012 23:31:59 +0200
> Added strict checking of PadN, as PadN can be used to increase header
> size and thus push the protocol header into the 2nd fragment.
>
> PadN is used to align the options within the Hop-by-Hop or
> Destination Options header to 64-bit boundaries. The maximum valid
> size is thus 7 bytes.
> RFC 4942 recommends to actively check the "payload" itself and
> ensure that it contains only zeroes.
>
> See also RFC 4942 section 2.1.9.5.
>
> Signed-off-by: Eldad Zack <eldad@fogrefinery.com>
Applied after fixing up the comment formatting.
Thanks.
^ permalink raw reply
* Re: [PATCH] netfilter: ipvs: Verify that IP_VS protocol has been registered
From: Simon Horman @ 2012-04-12 21:41 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Sasha Levin, wensong, ja, kaber, davem, davej, netdev, lvs-devel,
netfilter-devel, linux-kernel
In-Reply-To: <20120411234628.GA7253@1984>
On Thu, Apr 12, 2012 at 01:46:28AM +0200, Pablo Neira Ayuso wrote:
> On Thu, Apr 12, 2012 at 01:22:43AM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Apr 05, 2012 at 07:24:33PM -0400, Sasha Levin wrote:
> > > The registration of a protocol might fail, there were no checks
> > > and all registrations were assumed to be correct. This lead to
> > > NULL ptr dereferences when apps tried registering.
> > >
> > > For example:
> > >
> > > [ 1293.226051] BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
> > > [ 1293.227038] IP: [<ffffffff822aacb0>] tcp_register_app+0x60/0xb0
> > > [ 1293.227038] PGD 391de067 PUD 6c20b067 PMD 0
> > > [ 1293.227038] Oops: 0000 [#1] PREEMPT SMP
> > > [ 1293.227038] CPU 1
> > > [ 1293.227038] Pid: 19609, comm: trinity Tainted: G W 3.4.0-rc1-next-20120405-sasha-dirty #57
> > > [ 1293.227038] RIP: 0010:[<ffffffff822aacb0>] [<ffffffff822aacb0>] tcp_register_app+0x60/0xb0
> > > [ 1293.227038] RSP: 0018:ffff880038c1dd18 EFLAGS: 00010286
> > > [ 1293.227038] RAX: ffffffffffffffc0 RBX: 0000000000001500 RCX: 0000000000010000
> > > [ 1293.227038] RDX: 0000000000000000 RSI: ffff88003a2d5888 RDI: 0000000000000282
> > > [ 1293.227038] RBP: ffff880038c1dd48 R08: 0000000000000000 R09: 0000000000000000
> > > [ 1293.227038] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88003a2d5668
> > > [ 1293.227038] R13: ffff88003a2d5988 R14: ffff8800696a8ff8 R15: 0000000000000000
> > > [ 1293.227038] FS: 00007f01930d9700(0000) GS:ffff88007ce00000(0000) knlGS:0000000000000000
> > > [ 1293.227038] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > > [ 1293.227038] CR2: 0000000000000018 CR3: 0000000065dfc000 CR4: 00000000000406e0
> > > [ 1293.227038] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > [ 1293.227038] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > > [ 1293.227038] Process trinity (pid: 19609, threadinfo ffff880038c1c000, task ffff88002dc73000)
> > > [ 1293.227038] Stack:
> > > [ 1293.227038] ffff880038c1dd48 00000000fffffff4 ffff8800696aada0 ffff8800694f5580
> > > [ 1293.227038] ffffffff8369f1e0 0000000000001500 ffff880038c1dd98 ffffffff822a716b
> > > [ 1293.227038] 0000000000000000 ffff8800696a8ff8 0000000000000015 ffff8800694f5580
> > > [ 1293.227038] Call Trace:
> > > [ 1293.227038] [<ffffffff822a716b>] ip_vs_app_inc_new+0xdb/0x180
> > > [ 1293.227038] [<ffffffff822a7258>] register_ip_vs_app_inc+0x48/0x70
> > > [ 1293.227038] [<ffffffff822b2fea>] __ip_vs_ftp_init+0xba/0x140
> > > [ 1293.227038] [<ffffffff821c9060>] ops_init+0x80/0x90
> > > [ 1293.227038] [<ffffffff821c90cb>] setup_net+0x5b/0xe0
> > > [ 1293.227038] [<ffffffff821c9416>] copy_net_ns+0x76/0x100
> > > [ 1293.227038] [<ffffffff810dc92b>] create_new_namespaces+0xfb/0x190
> > > [ 1293.227038] [<ffffffff810dca21>] unshare_nsproxy_namespaces+0x61/0x80
> > > [ 1293.227038] [<ffffffff810afd1f>] sys_unshare+0xff/0x290
> > > [ 1293.227038] [<ffffffff8187622e>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> > > [ 1293.227038] [<ffffffff82665539>] system_call_fastpath+0x16/0x1b
> > > [ 1293.227038] Code: 89 c7 e8 34 91 3b 00 89 de 66 c1 ee 04 31 de 83 e6 0f 48 83 c6 22 48 c1 e6 04 4a 8b 14 26 49 8d 34 34 48 8d 42 c0 48 39 d6 74 13 <66> 39 58 58 74 22 48 8b 48 40 48 8d 41 c0 48 39 ce 75 ed 49 8d
> > > [ 1293.227038] RIP [<ffffffff822aacb0>] tcp_register_app+0x60/0xb0
> > > [ 1293.227038] RSP <ffff880038c1dd18>
> > > [ 1293.227038] CR2: 0000000000000018
> > > [ 1293.379284] ---[ end trace 364ab40c7011a009 ]---
> > > [ 1293.381182] Kernel panic - not syncing: Fatal exception in interrupt
> > >
> > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > > ---
> > > net/netfilter/ipvs/ip_vs_proto.c | 21 ++++++++++++++++-----
> > > 1 files changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
> > > index f843a88..43893ba 100644
> > > --- a/net/netfilter/ipvs/ip_vs_proto.c
> > > +++ b/net/netfilter/ipvs/ip_vs_proto.c
> > > @@ -316,20 +316,31 @@ ip_vs_tcpudp_debug_packet(int af, struct ip_vs_protocol *pp,
> > > */
> > > int __net_init ip_vs_protocol_net_init(struct net *net)
> > > {
> > > + int ret;
> > > #ifdef CONFIG_IP_VS_PROTO_TCP
> > > - register_ip_vs_proto_netns(net, &ip_vs_protocol_tcp);
> > > + ret = register_ip_vs_proto_netns(net, &ip_vs_protocol_tcp);
> > > + if (ret < 0)
> > > + return ret;
> > > #endif
> > > #ifdef CONFIG_IP_VS_PROTO_UDP
> > > - register_ip_vs_proto_netns(net, &ip_vs_protocol_udp);
> > > + ret = register_ip_vs_proto_netns(net, &ip_vs_protocol_udp);
> > > + if (ret < 0)
> > > + return ret;
> >
> > If you return here, I think you'll leave things in inconsistent state,
> > ie. the tcp protocol is registered. You have to unregister it before
> > leaving.
> >
> > ret = register_ip_vs_proto_netns(net, &ip_vs_protocol_tcp);
> ^^^
> sorry, I meant to say udp here.
>
> > if (ret < 0)
> > goto err_tcp;
> ^^^
> and here.
>
> > ...
> > err_tcp:
> > unregister_ip_vs_proto_netns(net, &ip_vs_protocol_tcp);
>
Thanks Pablo,
I was thinking along those lines too.
I'll send an updated patch.
^ permalink raw reply
* [PATCH v18 01/15] Add PR_{GET,SET}_NO_NEW_PRIVS to prevent execve from granting privs
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Andy Lutomirski, Will Drewry
From: Andy Lutomirski <luto@amacapital.net>
With this change, calling
prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)
disables privilege granting operations at execve-time. For example, a
process will not be able to execute a setuid binary to change their uid
or gid if this bit is set. The same is true for file capabilities.
Additionally, LSM_UNSAFE_NO_NEW_PRIVS is defined to ensure that
LSMs respect the requested behavior.
To determine if the NO_NEW_PRIVS bit is set, a task may call
prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
It returns 1 if set and 0 if it is not set. If any of the arguments are
non-zero, it will return -1 and set errno to -EINVAL.
(PR_SET_NO_NEW_PRIVS behaves similarly.)
This functionality is desired for the proposed seccomp filter patch
series. By using PR_SET_NO_NEW_PRIVS, it allows a task to modify the
system call behavior for itself and its child tasks without being
able to impact the behavior of a more privileged task.
Another potential use is making certain privileged operations
unprivileged. For example, chroot may be considered "safe" if it cannot
affect privileged tasks.
Note, this patch causes execve to fail when PR_SET_NO_NEW_PRIVS is
set and AppArmor is in use. It is fixed in a subsequent patch.
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Will Drewry <wad@chromium.org>
Acked-by: Eric Paris <eparis@redhat.com>
v18: updated change desc
v17: using new define values as per 3.4
---
fs/exec.c | 10 +++++++++-
include/linux/prctl.h | 15 +++++++++++++++
include/linux/sched.h | 2 ++
include/linux/security.h | 1 +
kernel/sys.c | 10 ++++++++++
security/apparmor/domain.c | 4 ++++
security/commoncap.c | 7 +++++--
security/selinux/hooks.c | 10 +++++++++-
8 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index b1fd202..d038968 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1245,6 +1245,13 @@ static int check_unsafe_exec(struct linux_binprm *bprm)
bprm->unsafe |= LSM_UNSAFE_PTRACE;
}
+ /*
+ * This isn't strictly necessary, but it makes it harder for LSMs to
+ * mess up.
+ */
+ if (current->no_new_privs)
+ bprm->unsafe |= LSM_UNSAFE_NO_NEW_PRIVS;
+
n_fs = 1;
spin_lock(&p->fs->lock);
rcu_read_lock();
@@ -1288,7 +1295,8 @@ int prepare_binprm(struct linux_binprm *bprm)
bprm->cred->euid = current_euid();
bprm->cred->egid = current_egid();
- if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)) {
+ if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) &&
+ !current->no_new_privs) {
/* Set-uid? */
if (mode & S_ISUID) {
bprm->per_clear |= PER_CLEAR_ON_SETID;
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index e0cfec2..78b76e2 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -124,4 +124,19 @@
#define PR_SET_CHILD_SUBREAPER 36
#define PR_GET_CHILD_SUBREAPER 37
+/*
+ * If no_new_privs is set, then operations that grant new privileges (i.e.
+ * execve) will either fail or not grant them. This affects suid/sgid,
+ * file capabilities, and LSMs.
+ *
+ * Operations that merely manipulate or drop existing privileges (setresuid,
+ * capset, etc.) will still work. Drop those privileges if you want them gone.
+ *
+ * Changing LSM security domain is considered a new privilege. So, for example,
+ * asking selinux for a specific new context (e.g. with runcon) will result
+ * in execve returning -EPERM.
+ */
+#define PR_SET_NO_NEW_PRIVS 38
+#define PR_GET_NO_NEW_PRIVS 39
+
#endif /* _LINUX_PRCTL_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 81a173c..ba60897 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1341,6 +1341,8 @@ struct task_struct {
* execve */
unsigned in_iowait:1;
+ /* task may not gain privileges */
+ unsigned no_new_privs:1;
/* Revert to default priority/policy when forking */
unsigned sched_reset_on_fork:1;
diff --git a/include/linux/security.h b/include/linux/security.h
index 673afbb..6e1dea9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -144,6 +144,7 @@ struct request_sock;
#define LSM_UNSAFE_SHARE 1
#define LSM_UNSAFE_PTRACE 2
#define LSM_UNSAFE_PTRACE_CAP 4
+#define LSM_UNSAFE_NO_NEW_PRIVS 8
#ifdef CONFIG_MMU
extern int mmap_min_addr_handler(struct ctl_table *table, int write,
diff --git a/kernel/sys.c b/kernel/sys.c
index e7006eb..b82568b 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1979,6 +1979,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
error = put_user(me->signal->is_child_subreaper,
(int __user *) arg2);
break;
+ case PR_SET_NO_NEW_PRIVS:
+ if (arg2 != 1 || arg3 || arg4 || arg5)
+ return -EINVAL;
+
+ current->no_new_privs = 1;
+ break;
+ case PR_GET_NO_NEW_PRIVS:
+ if (arg2 || arg3 || arg4 || arg5)
+ return -EINVAL;
+ return current->no_new_privs ? 1 : 0;
default:
error = -EINVAL;
break;
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 6327685..18c88d0 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,6 +360,10 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
if (bprm->cred_prepared)
return 0;
+ /* XXX: no_new_privs is not usable with AppArmor yet */
+ if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
+ return -EPERM;
+
cxt = bprm->cred->security;
BUG_ON(!cxt);
diff --git a/security/commoncap.c b/security/commoncap.c
index 0cf4b53..edd3918 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -506,14 +506,17 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
skip:
/* Don't let someone trace a set[ug]id/setpcap binary with the revised
- * credentials unless they have the appropriate permit
+ * credentials unless they have the appropriate permit.
+ *
+ * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
*/
if ((new->euid != old->uid ||
new->egid != old->gid ||
!cap_issubset(new->cap_permitted, old->cap_permitted)) &&
bprm->unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
/* downgrade; they get no more than they had, and maybe less */
- if (!capable(CAP_SETUID)) {
+ if (!capable(CAP_SETUID) ||
+ (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)) {
new->euid = new->uid;
new->egid = new->gid;
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d85b793..0b06685 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2016,6 +2016,13 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
new_tsec->sid = old_tsec->exec_sid;
/* Reset exec SID on execve. */
new_tsec->exec_sid = 0;
+
+ /*
+ * Minimize confusion: if no_new_privs and a transition is
+ * explicitly requested, then fail the exec.
+ */
+ if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
+ return -EPERM;
} else {
/* Check for a default transition on this program. */
rc = security_transition_sid(old_tsec->sid, isec->sid,
@@ -2029,7 +2036,8 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
ad.selinux_audit_data = &sad;
ad.u.path = bprm->file->f_path;
- if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)
+ if ((bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) ||
+ (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS))
new_tsec->sid = old_tsec->sid;
if (new_tsec->sid == old_tsec->sid) {
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 02/15] Fix execve behavior apparmor for PR_{GET,SET}_NO_NEW_PRIVS
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, John Johansen, Will Drewry,
Andy Lutomirski
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
From: John Johansen <john.johansen@canonical.com>
Add support for AppArmor to explicitly fail requested domain transitions
if NO_NEW_PRIVS is set and the task is not unconfined.
Transitions from unconfined are still allowed because this always results
in a reduction of privileges.
Acked-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
v18: new acked-by, new description
---
security/apparmor/domain.c | 39 +++++++++++++++++++++++++++++++++++----
1 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 18c88d0..b81ea10 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -360,10 +360,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
if (bprm->cred_prepared)
return 0;
- /* XXX: no_new_privs is not usable with AppArmor yet */
- if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS)
- return -EPERM;
-
cxt = bprm->cred->security;
BUG_ON(!cxt);
@@ -398,6 +394,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
new_profile = find_attach(ns, &ns->base.profiles, name);
if (!new_profile)
goto cleanup;
+ /*
+ * NOTE: Domain transitions from unconfined are allowed
+ * even when no_new_privs is set because this aways results
+ * in a further reduction of permissions.
+ */
goto apply;
}
@@ -459,6 +460,16 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
/* fail exec */
error = -EACCES;
+ /*
+ * Policy has specified a domain transition, if no_new_privs then
+ * fail the exec.
+ */
+ if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) {
+ aa_put_profile(new_profile);
+ error = -EPERM;
+ goto cleanup;
+ }
+
if (!new_profile)
goto audit;
@@ -613,6 +624,14 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
const char *target = NULL, *info = NULL;
int error = 0;
+ /*
+ * Fail explicitly requested domain transitions if no_new_privs.
+ * There is no exception for unconfined as change_hat is not
+ * available.
+ */
+ if (current->no_new_privs)
+ return -EPERM;
+
/* released below */
cred = get_current_cred();
cxt = cred->security;
@@ -754,6 +773,18 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
cxt = cred->security;
profile = aa_cred_profile(cred);
+ /*
+ * Fail explicitly requested domain transitions if no_new_privs
+ * and not unconfined.
+ * Domain transitions from unconfined are allowed even when
+ * no_new_privs is set because this aways results in a reduction
+ * of permissions.
+ */
+ if (current->no_new_privs && !unconfined(profile)) {
+ put_cred(cred);
+ return -EPERM;
+ }
+
if (ns_name) {
/* released below */
ns = aa_find_namespace(profile->ns, ns_name);
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 04/15] net/compat.c,linux/filter.h: share compat_sock_fprog
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: linux-man-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-arch-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8,
netdev-u79uwXL29TY76Z2rM5mHXA, x86-DgEjT+Ai2ygdnm+yROfE0A,
arnd-r2nGTMty4D4, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
hpa-YMNOUZJC4hwAvxtiuMwx3w, mingo-H+wXaHxf7aLQT0dZR+AlfA,
oleg-H+wXaHxf7aLQT0dZR+AlfA, peterz-wEGCiKHe2LqWVfeAwA7xHQ,
rdunlap-/UHa2rfvQTnk1uMJSBkQmQ, mcgrathr-F7+t8E8rja9g9hUCZPvPmw,
tglx-hfZtesqFncYOwBW4kG4KsQ, luto-3s7WtUTddSA,
eparis-H+wXaHxf7aLQT0dZR+AlfA,
serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw, djm-ilwOsaqNJrtAfugRpC6u6w,
scarybeasts-Re5JQEeQqe8AvxtiuMwx3w, indan-1J6HnF7K7zE,
pmoore-H+wXaHxf7aLQT0dZR+AlfA,
akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b, corbet-T1hC0tSOHrs,
eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w,
markus-F7+t8E8rja9g9hUCZPvPmw,
coreyb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
keescook-F7+t8E8rja9g9hUCZPvPmw, jmorris-gx6/JNMH7DfYtjvyW6yDsg,
Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Any other users of bpf_*_filter that take a struct sock_fprog from
userspace will need to be able to also accept a compat_sock_fprog
if the arch supports compat calls. This change allows the existing
compat_sock_fprog be shared.
Signed-off-by: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Acked-by: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Eric Paris <eparis-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
v18: tasered by the apostrophe police
v14: rebase/nochanges
v13: rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
v12: rebase on to linux-next
v11: introduction
---
include/linux/filter.h | 11 +++++++++++
net/compat.c | 8 --------
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/include/linux/filter.h b/include/linux/filter.h
index aaa2e80..f2e5315 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -10,6 +10,7 @@
#ifdef __KERNEL__
#include <linux/atomic.h>
+#include <linux/compat.h>
#endif
/*
@@ -132,6 +133,16 @@ struct sock_fprog { /* Required for SO_ATTACH_FILTER. */
#ifdef __KERNEL__
+#ifdef CONFIG_COMPAT
+/*
+ * A struct sock_filter is architecture independent.
+ */
+struct compat_sock_fprog {
+ u16 len;
+ compat_uptr_t filter; /* struct sock_filter * */
+};
+#endif
+
struct sk_buff;
struct sock;
diff --git a/net/compat.c b/net/compat.c
index e055708..242c828 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -328,14 +328,6 @@ void scm_detach_fds_compat(struct msghdr *kmsg, struct scm_cookie *scm)
__scm_destroy(scm);
}
-/*
- * A struct sock_filter is architecture independent.
- */
-struct compat_sock_fprog {
- u16 len;
- compat_uptr_t filter; /* struct sock_filter * */
-};
^ permalink raw reply related
* [PATCH v18 05/15] seccomp: kill the seccomp_t typedef
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
Replaces the seccomp_t typedef with struct seccomp to match modern
kernel style.
Signed-off-by: Will Drewry <wad@chromium.org>
Reviewed-by: James Morris <jmorris@namei.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Acked-by: Eric Paris <eparis@redhat.com>
v18: rebase
...
v14: rebase/nochanges
v13: rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
v12: rebase on to linux-next
v8-v11: no changes
v7: struct seccomp_struct -> struct seccomp
v6: original inclusion in this series.
---
include/linux/sched.h | 2 +-
include/linux/seccomp.h | 10 ++++++----
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ba60897..cad1502 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1452,7 +1452,7 @@ struct task_struct {
uid_t loginuid;
unsigned int sessionid;
#endif
- seccomp_t seccomp;
+ struct seccomp seccomp;
/* Thread group tracking */
u32 parent_exec_id;
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index cc7a4e9..d61f27f 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -7,7 +7,9 @@
#include <linux/thread_info.h>
#include <asm/seccomp.h>
-typedef struct { int mode; } seccomp_t;
+struct seccomp {
+ int mode;
+};
extern void __secure_computing(int);
static inline void secure_computing(int this_syscall)
@@ -19,7 +21,7 @@ static inline void secure_computing(int this_syscall)
extern long prctl_get_seccomp(void);
extern long prctl_set_seccomp(unsigned long);
-static inline int seccomp_mode(seccomp_t *s)
+static inline int seccomp_mode(struct seccomp *s)
{
return s->mode;
}
@@ -28,7 +30,7 @@ static inline int seccomp_mode(seccomp_t *s)
#include <linux/errno.h>
-typedef struct { } seccomp_t;
+struct seccomp { };
#define secure_computing(x) do { } while (0)
@@ -42,7 +44,7 @@ static inline long prctl_set_seccomp(unsigned long arg2)
return -EINVAL;
}
-static inline int seccomp_mode(seccomp_t *s)
+static inline int seccomp_mode(struct seccomp *s)
{
return 0;
}
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 06/15] asm/syscall.h: add syscall_get_arch
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
Adds a stub for a function that will return the AUDIT_ARCH_* value
appropriate to the supplied task based on the system call convention.
For audit's use, the value can generally be hard-coded at the
audit-site. However, for other functionality not inlined into syscall
entry/exit, this makes that information available. seccomp_filter is
the first planned consumer and, as such, the comment indicates a tie to
CONFIG_HAVE_ARCH_SECCOMP_FILTER.
Suggested-by: Roland McGrath <mcgrathr@chromium.org>
Signed-off-by: Will Drewry <wad@chromium.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Acked-by: Eric Paris <eparis@redhat.com>
v18: comment and change reword and rebase.
v14: rebase/nochanges
v13: rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
v12: rebase on to linux-next
v11: fixed improper return type
v10: introduced
---
include/asm-generic/syscall.h | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
index 5c122ae..5b09392 100644
--- a/include/asm-generic/syscall.h
+++ b/include/asm-generic/syscall.h
@@ -142,4 +142,18 @@ void syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
unsigned int i, unsigned int n,
const unsigned long *args);
+/**
+ * syscall_get_arch - return the AUDIT_ARCH for the current system call
+ * @task: task of interest, must be in system call entry tracing
+ * @regs: task_pt_regs() of @task
+ *
+ * Returns the AUDIT_ARCH_* based on the system call convention in use.
+ *
+ * It's only valid to call this when @task is stopped on entry to a system
+ * call, due to %TIF_SYSCALL_TRACE, %TIF_SYSCALL_AUDIT, or %TIF_SECCOMP.
+ *
+ * Architectures which permit CONFIG_HAVE_ARCH_SECCOMP_FILTER must
+ * provide an implementation of this.
+ */
+int syscall_get_arch(struct task_struct *task, struct pt_regs *regs);
#endif /* _ASM_SYSCALL_H */
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 07/15] arch/x86: add syscall_get_arch to syscall.h
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
Add syscall_get_arch() to export the current AUDIT_ARCH_* based on system call
entry path.
Signed-off-by: Will Drewry <wad@chromium.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: H. Peter Anvin <hpa@zytor.com>
Acked-by: Eric Paris <eparis@redhat.com>
v18: - update comment about x32 tasks
- rebase to v3.4-rc2
v17: rebase and reviewed-by
v14: rebase/nochanges
v13: rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
---
arch/x86/include/asm/syscall.h | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 386b786..1ace47b 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -13,9 +13,11 @@
#ifndef _ASM_X86_SYSCALL_H
#define _ASM_X86_SYSCALL_H
+#include <linux/audit.h>
#include <linux/sched.h>
#include <linux/err.h>
#include <asm/asm-offsets.h> /* For NR_syscalls */
+#include <asm/thread_info.h> /* for TS_COMPAT */
#include <asm/unistd.h>
extern const unsigned long sys_call_table[];
@@ -88,6 +90,12 @@ static inline void syscall_set_arguments(struct task_struct *task,
memcpy(®s->bx + i, args, n * sizeof(args[0]));
}
+static inline int syscall_get_arch(struct task_struct *task,
+ struct pt_regs *regs)
+{
+ return AUDIT_ARCH_I386;
+}
+
#else /* CONFIG_X86_64 */
static inline void syscall_get_arguments(struct task_struct *task,
@@ -212,6 +220,25 @@ static inline void syscall_set_arguments(struct task_struct *task,
}
}
+static inline int syscall_get_arch(struct task_struct *task,
+ struct pt_regs *regs)
+{
+#ifdef CONFIG_IA32_EMULATION
+ /*
+ * TS_COMPAT is set for 32-bit syscall entry and then
+ * remains set until we return to user mode.
+ *
+ * TIF_IA32 tasks should always have TS_COMPAT set at
+ * system call time.
+ *
+ * x32 tasks should be considered AUDIT_ARCH_X86_64.
+ */
+ if (task_thread_info(task)->status & TS_COMPAT)
+ return AUDIT_ARCH_I386;
+#endif
+ /* Both x32 and x86_64 are considered "64-bit". */
+ return AUDIT_ARCH_X86_64;
+}
#endif /* CONFIG_X86_32 */
#endif /* _ASM_X86_SYSCALL_H */
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 08/15] seccomp: add system call filtering using BPF
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
[This patch depends on luto@mit.edu's no_new_privs patch:
https://lkml.org/lkml/2012/1/30/264
The whole series including Andrew's patches can be found here:
https://github.com/redpig/linux/tree/seccomp
Complete diff here:
https://github.com/redpig/linux/compare/1dc65fed...seccomp
]
This patch adds support for seccomp mode 2. Mode 2 introduces the
ability for unprivileged processes to install system call filtering
policy expressed in terms of a Berkeley Packet Filter (BPF) program.
This program will be evaluated in the kernel for each system call
the task makes and computes a result based on data in the format
of struct seccomp_data.
A filter program may be installed by calling:
struct sock_fprog fprog = { ... };
...
prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &fprog);
The return value of the filter program determines if the system call is
allowed to proceed or denied. If the first filter program installed
allows prctl(2) calls, then the above call may be made repeatedly
by a task to further reduce its access to the kernel. All attached
programs must be evaluated before a system call will be allowed to
proceed.
Filter programs will be inherited across fork/clone and execve.
However, if the task attaching the filter is unprivileged
(!CAP_SYS_ADMIN) the no_new_privs bit will be set on the task. This
ensures that unprivileged tasks cannot attach filters that affect
privileged tasks (e.g., setuid binary).
There are a number of benefits to this approach. A few of which are
as follows:
- BPF has been exposed to userland for a long time
- BPF optimization (and JIT'ing) are well understood
- Userland already knows its ABI: system call numbers and desired
arguments
- No time-of-check-time-of-use vulnerable data accesses are possible.
- system call arguments are loaded on access only to minimize copying
required for system call policy decisions.
Mode 2 support is restricted to architectures that enable
HAVE_ARCH_SECCOMP_FILTER. In this patch, the primary dependency is on
syscall_get_arguments(). The full desired scope of this feature will
add a few minor additional requirements expressed later in this series.
Based on discussion, SECCOMP_RET_ERRNO and SECCOMP_RET_TRACE seem to be
the desired additional functionality.
No architectures are enabled in this patch.
Signed-off-by: Will Drewry <wad@chromium.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: Indan Zupancic <indan@nul.nu>
Acked-by: Eric Paris <eparis@redhat.com>
v18: - rebase to v3.4-rc2
- s/chk/check/ (akpm@linux-foundation.org,jmorris@namei.org)
- allocate with GFP_KERNEL|__GFP_NOWARN (indan@nul.nu)
- add a comment for get_u32 regarding endianness (akpm@)
- fix other typos, style mistakes (akpm@)
- added acked-by
v17: - properly guard seccomp filter needed headers (leann@ubuntu.com)
- tighten return mask to 0x7fff0000
v16: - no change
v15: - add a 4 instr penalty when counting a path to account for seccomp_filter
size (indan@nul.nu)
- drop the max insns to 256KB (indan@nul.nu)
- return ENOMEM if the max insns limit has been hit (indan@nul.nu)
- move IP checks after args (indan@nul.nu)
- drop !user_filter check (indan@nul.nu)
- only allow explicit bpf codes (indan@nul.nu)
- exit_code -> exit_sig
v14: - put/get_seccomp_filter takes struct task_struct
(indan@nul.nu,keescook@chromium.org)
- adds seccomp_chk_filter and drops general bpf_run/chk_filter user
- add seccomp_bpf_load for use by net/core/filter.c
- lower max per-process/per-hierarchy: 1MB
- moved nnp/capability check prior to allocation
(all of the above: indan@nul.nu)
v13: - rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
v12: - added a maximum instruction count per path (indan@nul.nu,oleg@redhat.com)
- removed copy_seccomp (keescook@chromium.org,indan@nul.nu)
- reworded the prctl_set_seccomp comment (indan@nul.nu)
v11: - reorder struct seccomp_data to allow future args expansion (hpa@zytor.com)
- style clean up, @compat dropped, compat_sock_fprog32 (indan@nul.nu)
- do_exit(SIGSYS) (keescook@chromium.org, luto@mit.edu)
- pare down Kconfig doc reference.
- extra comment clean up
v10: - seccomp_data has changed again to be more aesthetically pleasing
(hpa@zytor.com)
- calling convention is noted in a new u32 field using syscall_get_arch.
This allows for cross-calling convention tasks to use seccomp filters.
(hpa@zytor.com)
- lots of clean up (thanks, Indan!)
v9: - n/a
v8: - use bpf_chk_filter, bpf_run_filter. update load_fns
- Lots of fixes courtesy of indan@nul.nu:
-- fix up load behavior, compat fixups, and merge alloc code,
-- renamed pc and dropped __packed, use bool compat.
-- Added a hidden CONFIG_SECCOMP_FILTER to synthesize non-arch
dependencies
v7: (massive overhaul thanks to Indan, others)
- added CONFIG_HAVE_ARCH_SECCOMP_FILTER
- merged into seccomp.c
- minimal seccomp_filter.h
- no config option (part of seccomp)
- no new prctl
- doesn't break seccomp on systems without asm/syscall.h
(works but arg access always fails)
- dropped seccomp_init_task, extra free functions, ...
- dropped the no-asm/syscall.h code paths
- merges with network sk_run_filter and sk_chk_filter
v6: - fix memory leak on attach compat check failure
- require no_new_privs || CAP_SYS_ADMIN prior to filter
installation. (luto@mit.edu)
- s/seccomp_struct_/seccomp_/ for macros/functions (amwang@redhat.com)
- cleaned up Kconfig (amwang@redhat.com)
- on block, note if the call was compat (so the # means something)
v5: - uses syscall_get_arguments
(indan@nul.nu,oleg@redhat.com, mcgrathr@chromium.org)
- uses union-based arg storage with hi/lo struct to
handle endianness. Compromises between the two alternate
proposals to minimize extra arg shuffling and account for
endianness assuming userspace uses offsetof().
(mcgrathr@chromium.org, indan@nul.nu)
- update Kconfig description
- add include/seccomp_filter.h and add its installation
- (naive) on-demand syscall argument loading
- drop seccomp_t (eparis@redhat.com)
v4: - adjusted prctl to make room for PR_[SG]ET_NO_NEW_PRIVS
- now uses current->no_new_privs
(luto@mit.edu,torvalds@linux-foundation.com)
- assign names to seccomp modes (rdunlap@xenotime.net)
- fix style issues (rdunlap@xenotime.net)
- reworded Kconfig entry (rdunlap@xenotime.net)
v3: - macros to inline (oleg@redhat.com)
- init_task behavior fixed (oleg@redhat.com)
- drop creator entry and extra NULL check (oleg@redhat.com)
- alloc returns -EINVAL on bad sizing (serge.hallyn@canonical.com)
- adds tentative use of "always_unprivileged" as per
torvalds@linux-foundation.org and luto@mit.edu
v2: - (patch 2 only)
---
arch/Kconfig | 17 ++
include/linux/Kbuild | 1 +
include/linux/seccomp.h | 76 +++++++++-
kernel/fork.c | 3 +
kernel/seccomp.c | 396 ++++++++++++++++++++++++++++++++++++++++++++--
kernel/sys.c | 2 +-
6 files changed, 472 insertions(+), 23 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 684eb5a..91c2c73 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -216,4 +216,21 @@ config HAVE_CMPXCHG_DOUBLE
config ARCH_WANT_OLD_COMPAT_IPC
bool
+config HAVE_ARCH_SECCOMP_FILTER
+ bool
+ help
+ This symbol should be selected by an architecure if it provides
+ asm/syscall.h, specifically syscall_get_arguments() and
+ syscall_get_arch().
+
+config SECCOMP_FILTER
+ def_bool y
+ depends on HAVE_ARCH_SECCOMP_FILTER && SECCOMP && NET
+ help
+ Enable tasks to build secure computing environments defined
+ in terms of Berkeley Packet Filter programs which implement
+ task-defined system call filtering polices.
+
+ See Documentation/prctl/seccomp_filter.txt for details.
+
source "kernel/gcov/Kconfig"
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 3c9b616..5c93d6c 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -332,6 +332,7 @@ header-y += scc.h
header-y += sched.h
header-y += screen_info.h
header-y += sdla.h
+header-y += seccomp.h
header-y += securebits.h
header-y += selinux_netlink.h
header-y += sem.h
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index d61f27f..86bb68f 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -1,14 +1,67 @@
#ifndef _LINUX_SECCOMP_H
#define _LINUX_SECCOMP_H
+#include <linux/compiler.h>
+#include <linux/types.h>
+
+
+/* Valid values for seccomp.mode and prctl(PR_SET_SECCOMP, <mode>) */
+#define SECCOMP_MODE_DISABLED 0 /* seccomp is not in use. */
+#define SECCOMP_MODE_STRICT 1 /* uses hard-coded filter. */
+#define SECCOMP_MODE_FILTER 2 /* uses user-supplied filter. */
+
+/*
+ * All BPF programs must return a 32-bit value.
+ * The bottom 16-bits are reserved for future use.
+ * The upper 16-bits are ordered from least permissive values to most.
+ *
+ * The ordering ensures that a min_t() over composed return values always
+ * selects the least permissive choice.
+ */
+#define SECCOMP_RET_KILL 0x00000000U /* kill the task immediately */
+#define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
+
+/* Masks for the return value sections. */
+#define SECCOMP_RET_ACTION 0x7fff0000U
+#define SECCOMP_RET_DATA 0x0000ffffU
+
+/**
+ * struct seccomp_data - the format the BPF program executes over.
+ * @nr: the system call number
+ * @arch: indicates system call convention as an AUDIT_ARCH_* value
+ * as defined in <linux/audit.h>.
+ * @instruction_pointer: at the time of the system call.
+ * @args: up to 6 system call arguments always stored as 64-bit values
+ * regardless of the architecture.
+ */
+struct seccomp_data {
+ int nr;
+ __u32 arch;
+ __u64 instruction_pointer;
+ __u64 args[6];
+};
+#ifdef __KERNEL__
#ifdef CONFIG_SECCOMP
#include <linux/thread_info.h>
#include <asm/seccomp.h>
+struct seccomp_filter;
+/**
+ * struct seccomp - the state of a seccomp'ed process
+ *
+ * @mode: indicates one of the valid values above for controlled
+ * system calls available to a process.
+ * @filter: The metadata and ruleset for determining what system calls
+ * are allowed for a task.
+ *
+ * @filter must only be accessed from the context of current as there
+ * is no locking.
+ */
struct seccomp {
int mode;
+ struct seccomp_filter *filter;
};
extern void __secure_computing(int);
@@ -19,7 +72,7 @@ static inline void secure_computing(int this_syscall)
}
extern long prctl_get_seccomp(void);
-extern long prctl_set_seccomp(unsigned long);
+extern long prctl_set_seccomp(unsigned long, char __user *);
static inline int seccomp_mode(struct seccomp *s)
{
@@ -31,15 +84,16 @@ static inline int seccomp_mode(struct seccomp *s)
#include <linux/errno.h>
struct seccomp { };
+struct seccomp_filter { };
-#define secure_computing(x) do { } while (0)
+#define secure_computing(x) 0
static inline long prctl_get_seccomp(void)
{
return -EINVAL;
}
-static inline long prctl_set_seccomp(unsigned long arg2)
+static inline long prctl_set_seccomp(unsigned long arg2, char __user *arg3)
{
return -EINVAL;
}
@@ -48,7 +102,21 @@ static inline int seccomp_mode(struct seccomp *s)
{
return 0;
}
-
#endif /* CONFIG_SECCOMP */
+#ifdef CONFIG_SECCOMP_FILTER
+extern void put_seccomp_filter(struct task_struct *tsk);
+extern void get_seccomp_filter(struct task_struct *tsk);
+extern u32 seccomp_bpf_load(int off);
+#else /* CONFIG_SECCOMP_FILTER */
+static inline void put_seccomp_filter(struct task_struct *tsk)
+{
+ return;
+}
+static inline void get_seccomp_filter(struct task_struct *tsk)
+{
+ return;
+}
+#endif /* CONFIG_SECCOMP_FILTER */
+#endif /* __KERNEL__ */
#endif /* _LINUX_SECCOMP_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index b9372a0..f7cf6fb 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -34,6 +34,7 @@
#include <linux/cgroup.h>
#include <linux/security.h>
#include <linux/hugetlb.h>
+#include <linux/seccomp.h>
#include <linux/swap.h>
#include <linux/syscalls.h>
#include <linux/jiffies.h>
@@ -170,6 +171,7 @@ void free_task(struct task_struct *tsk)
free_thread_info(tsk->stack);
rt_mutex_debug_task_free(tsk);
ftrace_graph_exit_task(tsk);
+ put_seccomp_filter(tsk);
free_task_struct(tsk);
}
EXPORT_SYMBOL(free_task);
@@ -1162,6 +1164,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
goto fork_out;
ftrace_graph_init_task(p);
+ get_seccomp_filter(p);
rt_mutex_init_task(p);
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index e8d76c5..0aeec19 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -3,16 +3,343 @@
*
* Copyright 2004-2005 Andrea Arcangeli <andrea@cpushare.com>
*
- * This defines a simple but solid secure-computing mode.
+ * Copyright (C) 2012 Google, Inc.
+ * Will Drewry <wad@chromium.org>
+ *
+ * This defines a simple but solid secure-computing facility.
+ *
+ * Mode 1 uses a fixed list of allowed system calls.
+ * Mode 2 allows user-defined system call filters in the form
+ * of Berkeley Packet Filters/Linux Socket Filters.
*/
+#include <linux/atomic.h>
#include <linux/audit.h>
-#include <linux/seccomp.h>
-#include <linux/sched.h>
#include <linux/compat.h>
+#include <linux/sched.h>
+#include <linux/seccomp.h>
/* #define SECCOMP_DEBUG 1 */
-#define NR_SECCOMP_MODES 1
+
+#ifdef CONFIG_SECCOMP_FILTER
+#include <asm/syscall.h>
+#include <linux/filter.h>
+#include <linux/security.h>
+#include <linux/slab.h>
+#include <linux/tracehook.h>
+#include <linux/uaccess.h>
+
+/**
+ * struct seccomp_filter - container for seccomp BPF programs
+ *
+ * @usage: reference count to manage the object lifetime.
+ * get/put helpers should be used when accessing an instance
+ * outside of a lifetime-guarded section. In general, this
+ * is only needed for handling filters shared across tasks.
+ * @prev: points to a previously installed, or inherited, filter
+ * @len: the number of instructions in the program
+ * @insns: the BPF program instructions to evaluate
+ *
+ * seccomp_filter objects are organized in a tree linked via the @prev
+ * pointer. For any task, it appears to be a singly-linked list starting
+ * with current->seccomp.filter, the most recently attached or inherited filter.
+ * However, multiple filters may share a @prev node, by way of fork(), which
+ * results in a unidirectional tree existing in memory. This is similar to
+ * how namespaces work.
+ *
+ * seccomp_filter objects should never be modified after being attached
+ * to a task_struct (other than @usage).
+ */
+struct seccomp_filter {
+ atomic_t usage;
+ struct seccomp_filter *prev;
+ unsigned short len; /* Instruction count */
+ struct sock_filter insns[];
+};
+
+/* Limit any path through the tree to 256KB worth of instructions. */
+#define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
+
+static void seccomp_filter_log_failure(int syscall)
+{
+ int compat = 0;
+#ifdef CONFIG_COMPAT
+ compat = is_compat_task();
+#endif
+ pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n",
+ current->comm, task_pid_nr(current),
+ (compat ? "compat " : ""),
+ syscall, KSTK_EIP(current));
+}
+
+/**
+ * get_u32 - returns a u32 offset into data
+ * @data: a unsigned 64 bit value
+ * @index: 0 or 1 to return the first or second 32-bits
+ *
+ * This inline exists to hide the length of unsigned long. If a 32-bit
+ * unsigned long is passed in, it will be extended and the top 32-bits will be
+ * 0. If it is a 64-bit unsigned long, then whatever data is resident will be
+ * properly returned.
+ *
+ * Endianness is explicitly ignored and left for BPF program authors to manage
+ * as per the specific architecture.
+ */
+static inline u32 get_u32(u64 data, int index)
+{
+ return ((u32 *)&data)[index];
+}
+
+/* Helper for bpf_load below. */
+#define BPF_DATA(_name) offsetof(struct seccomp_data, _name)
+/**
+ * bpf_load: checks and returns a pointer to the requested offset
+ * @off: offset into struct seccomp_data to load from
+ *
+ * Returns the requested 32-bits of data.
+ * seccomp_check_filter() should assure that @off is 32-bit aligned
+ * and not out of bounds. Failure to do so is a BUG.
+ */
+u32 seccomp_bpf_load(int off)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+ if (off == BPF_DATA(nr))
+ return syscall_get_nr(current, regs);
+ if (off == BPF_DATA(arch))
+ return syscall_get_arch(current, regs);
+ if (off >= BPF_DATA(args[0]) && off < BPF_DATA(args[6])) {
+ unsigned long value;
+ int arg = (off - BPF_DATA(args[0])) / sizeof(u64);
+ int index = !!(off % sizeof(u64));
+ syscall_get_arguments(current, regs, arg, 1, &value);
+ return get_u32(value, index);
+ }
+ if (off == BPF_DATA(instruction_pointer))
+ return get_u32(KSTK_EIP(current), 0);
+ if (off == BPF_DATA(instruction_pointer) + sizeof(u32))
+ return get_u32(KSTK_EIP(current), 1);
+ /* seccomp_check_filter should make this impossible. */
+ BUG();
+}
+
+/**
+ * seccomp_check_filter - verify seccomp filter code
+ * @filter: filter to verify
+ * @flen: length of filter
+ *
+ * Takes a previously checked filter (by sk_chk_filter) and
+ * redirects all filter code that loads struct sk_buff data
+ * and related data through seccomp_bpf_load. It also
+ * enforces length and alignment checking of those loads.
+ *
+ * Returns 0 if the rule set is legal or -EINVAL if not.
+ */
+static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
+{
+ int pc;
+ for (pc = 0; pc < flen; pc++) {
+ struct sock_filter *ftest = &filter[pc];
+ u16 code = ftest->code;
+ u32 k = ftest->k;
+
+ switch (code) {
+ case BPF_S_LD_W_ABS:
+ ftest->code = BPF_S_ANC_SECCOMP_LD_W;
+ /* 32-bit aligned and not out of bounds. */
+ if (k >= sizeof(struct seccomp_data) || k & 3)
+ return -EINVAL;
+ continue;
+ case BPF_S_LD_W_LEN:
+ ftest->code = BPF_S_LD_IMM;
+ ftest->k = sizeof(struct seccomp_data);
+ continue;
+ case BPF_S_LDX_W_LEN:
+ ftest->code = BPF_S_LDX_IMM;
+ ftest->k = sizeof(struct seccomp_data);
+ continue;
+ /* Explicitly include allowed calls. */
+ case BPF_S_RET_K:
+ case BPF_S_RET_A:
+ case BPF_S_ALU_ADD_K:
+ case BPF_S_ALU_ADD_X:
+ case BPF_S_ALU_SUB_K:
+ case BPF_S_ALU_SUB_X:
+ case BPF_S_ALU_MUL_K:
+ case BPF_S_ALU_MUL_X:
+ case BPF_S_ALU_DIV_X:
+ case BPF_S_ALU_AND_K:
+ case BPF_S_ALU_AND_X:
+ case BPF_S_ALU_OR_K:
+ case BPF_S_ALU_OR_X:
+ case BPF_S_ALU_LSH_K:
+ case BPF_S_ALU_LSH_X:
+ case BPF_S_ALU_RSH_K:
+ case BPF_S_ALU_RSH_X:
+ case BPF_S_ALU_NEG:
+ case BPF_S_LD_IMM:
+ case BPF_S_LDX_IMM:
+ case BPF_S_MISC_TAX:
+ case BPF_S_MISC_TXA:
+ case BPF_S_ALU_DIV_K:
+ case BPF_S_LD_MEM:
+ case BPF_S_LDX_MEM:
+ case BPF_S_ST:
+ case BPF_S_STX:
+ case BPF_S_JMP_JA:
+ case BPF_S_JMP_JEQ_K:
+ case BPF_S_JMP_JEQ_X:
+ case BPF_S_JMP_JGE_K:
+ case BPF_S_JMP_JGE_X:
+ case BPF_S_JMP_JGT_K:
+ case BPF_S_JMP_JGT_X:
+ case BPF_S_JMP_JSET_K:
+ case BPF_S_JMP_JSET_X:
+ continue;
+ default:
+ return -EINVAL;
+ }
+ }
+ return 0;
+}
+
+/**
+ * seccomp_run_filters - evaluates all seccomp filters against @syscall
+ * @syscall: number of the current system call
+ *
+ * Returns valid seccomp BPF response codes.
+ */
+static u32 seccomp_run_filters(int syscall)
+{
+ struct seccomp_filter *f;
+ u32 ret = SECCOMP_RET_KILL;
+ /*
+ * All filters in the list are evaluated and the lowest BPF return
+ * value always takes priority.
+ */
+ for (f = current->seccomp.filter; f; f = f->prev) {
+ ret = sk_run_filter(NULL, f->insns);
+ if (ret != SECCOMP_RET_ALLOW)
+ break;
+ }
+ return ret;
+}
+
+/**
+ * seccomp_attach_filter: Attaches a seccomp filter to current.
+ * @fprog: BPF program to install
+ *
+ * Returns 0 on success or an errno on failure.
+ */
+static long seccomp_attach_filter(struct sock_fprog *fprog)
+{
+ struct seccomp_filter *filter;
+ unsigned long fp_size = fprog->len * sizeof(struct sock_filter);
+ unsigned long total_insns = fprog->len;
+ long ret;
+
+ if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
+ return -EINVAL;
+
+ for (filter = current->seccomp.filter; filter; filter = filter->prev)
+ total_insns += filter->len + 4; /* include a 4 instr penalty */
+ if (total_insns > MAX_INSNS_PER_PATH)
+ return -ENOMEM;
+
+ /*
+ * Installing a seccomp filter requires that the task have
+ * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
+ * This avoids scenarios where unprivileged tasks can affect the
+ * behavior of privileged children.
+ */
+ if (!current->no_new_privs &&
+ security_capable_noaudit(current_cred(), current_user_ns(),
+ CAP_SYS_ADMIN) != 0)
+ return -EACCES;
+
+ /* Allocate a new seccomp_filter */
+ filter = kzalloc(sizeof(struct seccomp_filter) + fp_size,
+ GFP_KERNEL|__GFP_NOWARN);
+ if (!filter)
+ return -ENOMEM;
+ atomic_set(&filter->usage, 1);
+ filter->len = fprog->len;
+
+ /* Copy the instructions from fprog. */
+ ret = -EFAULT;
+ if (copy_from_user(filter->insns, fprog->filter, fp_size))
+ goto fail;
+
+ /* Check and rewrite the fprog via the skb checker */
+ ret = sk_chk_filter(filter->insns, filter->len);
+ if (ret)
+ goto fail;
+
+ /* Check and rewrite the fprog for seccomp use */
+ ret = seccomp_check_filter(filter->insns, filter->len);
+ if (ret)
+ goto fail;
+
+ /*
+ * If there is an existing filter, make it the prev and don't drop its
+ * task reference.
+ */
+ filter->prev = current->seccomp.filter;
+ current->seccomp.filter = filter;
+ return 0;
+fail:
+ kfree(filter);
+ return ret;
+}
+
+/**
+ * seccomp_attach_user_filter - attaches a user-supplied sock_fprog
+ * @user_filter: pointer to the user data containing a sock_fprog.
+ *
+ * Returns 0 on success and non-zero otherwise.
+ */
+long seccomp_attach_user_filter(char __user *user_filter)
+{
+ struct sock_fprog fprog;
+ long ret = -EFAULT;
+
+#ifdef CONFIG_COMPAT
+ if (is_compat_task()) {
+ struct compat_sock_fprog fprog32;
+ if (copy_from_user(&fprog32, user_filter, sizeof(fprog32)))
+ goto out;
+ fprog.len = fprog32.len;
+ fprog.filter = compat_ptr(fprog32.filter);
+ } else /* falls through to the if below. */
+#endif
+ if (copy_from_user(&fprog, user_filter, sizeof(fprog)))
+ goto out;
+ ret = seccomp_attach_filter(&fprog);
+out:
+ return ret;
+}
+
+/* get_seccomp_filter - increments the reference count of the filter on @tsk */
+void get_seccomp_filter(struct task_struct *tsk)
+{
+ struct seccomp_filter *orig = tsk->seccomp.filter;
+ if (!orig)
+ return;
+ /* Reference count is bounded by the number of total processes. */
+ atomic_inc(&orig->usage);
+}
+
+/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
+void put_seccomp_filter(struct task_struct *tsk)
+{
+ struct seccomp_filter *orig = tsk->seccomp.filter;
+ /* Clean up single-reference branches iteratively. */
+ while (orig && atomic_dec_and_test(&orig->usage)) {
+ struct seccomp_filter *freeme = orig;
+ orig = orig->prev;
+ kfree(freeme);
+ }
+}
+#endif /* CONFIG_SECCOMP_FILTER */
/*
* Secure computing mode 1 allows only read/write/exit/sigreturn.
@@ -34,10 +361,11 @@ static int mode1_syscalls_32[] = {
void __secure_computing(int this_syscall)
{
int mode = current->seccomp.mode;
- int * syscall;
+ int exit_sig = 0;
+ int *syscall;
switch (mode) {
- case 1:
+ case SECCOMP_MODE_STRICT:
syscall = mode1_syscalls;
#ifdef CONFIG_COMPAT
if (is_compat_task())
@@ -47,7 +375,16 @@ void __secure_computing(int this_syscall)
if (*syscall == this_syscall)
return;
} while (*++syscall);
+ exit_sig = SIGKILL;
break;
+#ifdef CONFIG_SECCOMP_FILTER
+ case SECCOMP_MODE_FILTER:
+ if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
+ return;
+ seccomp_filter_log_failure(this_syscall);
+ exit_sig = SIGSYS;
+ break;
+#endif
default:
BUG();
}
@@ -56,7 +393,7 @@ void __secure_computing(int this_syscall)
dump_stack();
#endif
audit_seccomp(this_syscall);
- do_exit(SIGKILL);
+ do_exit(exit_sig);
}
long prctl_get_seccomp(void)
@@ -64,25 +401,48 @@ long prctl_get_seccomp(void)
return current->seccomp.mode;
}
-long prctl_set_seccomp(unsigned long seccomp_mode)
+/**
+ * prctl_set_seccomp: configures current->seccomp.mode
+ * @seccomp_mode: requested mode to use
+ * @filter: optional struct sock_fprog for use with SECCOMP_MODE_FILTER
+ *
+ * This function may be called repeatedly with a @seccomp_mode of
+ * SECCOMP_MODE_FILTER to install additional filters. Every filter
+ * successfully installed will be evaluated (in reverse order) for each system
+ * call the task makes.
+ *
+ * Once current->seccomp.mode is non-zero, it may not be changed.
+ *
+ * Returns 0 on success or -EINVAL on failure.
+ */
+long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
{
- long ret;
+ long ret = -EINVAL;
- /* can set it only once to be even more secure */
- ret = -EPERM;
- if (unlikely(current->seccomp.mode))
+ if (current->seccomp.mode &&
+ current->seccomp.mode != seccomp_mode)
goto out;
- ret = -EINVAL;
- if (seccomp_mode && seccomp_mode <= NR_SECCOMP_MODES) {
- current->seccomp.mode = seccomp_mode;
- set_thread_flag(TIF_SECCOMP);
+ switch (seccomp_mode) {
+ case SECCOMP_MODE_STRICT:
+ ret = 0;
#ifdef TIF_NOTSC
disable_TSC();
#endif
- ret = 0;
+ break;
+#ifdef CONFIG_SECCOMP_FILTER
+ case SECCOMP_MODE_FILTER:
+ ret = seccomp_attach_user_filter(filter);
+ if (ret)
+ goto out;
+ break;
+#endif
+ default:
+ goto out;
}
- out:
+ current->seccomp.mode = seccomp_mode;
+ set_thread_flag(TIF_SECCOMP);
+out:
return ret;
}
diff --git a/kernel/sys.c b/kernel/sys.c
index b82568b..ba0ae8e 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1908,7 +1908,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
error = prctl_get_seccomp();
break;
case PR_SET_SECCOMP:
- error = prctl_set_seccomp(arg2);
+ error = prctl_set_seccomp(arg2, (char __user *)arg3);
break;
case PR_GET_TSC:
error = GET_TSC_CTL(arg2);
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 09/15] seccomp: remove duplicated failure logging
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
From: Kees Cook <keescook@chromium.org>
This consolidates the seccomp filter error logging path and adds more
details to the audit log.
Signed-off-by: Will Drewry <wad@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Eric Paris <eparis@redhat.com>
v18: make compat= permanent in the record
v15: added a return code to the audit_seccomp path by wad@chromium.org
(suggested by eparis@redhat.com)
v*: original by keescook@chromium.org
---
include/linux/audit.h | 8 ++++----
kernel/auditsc.c | 8 ++++++--
kernel/seccomp.c | 15 +--------------
3 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index ed3ef19..22f292a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -463,7 +463,7 @@ extern void audit_putname(const char *name);
extern void __audit_inode(const char *name, const struct dentry *dentry);
extern void __audit_inode_child(const struct dentry *dentry,
const struct inode *parent);
-extern void __audit_seccomp(unsigned long syscall);
+extern void __audit_seccomp(unsigned long syscall, long signr, int code);
extern void __audit_ptrace(struct task_struct *t);
static inline int audit_dummy_context(void)
@@ -508,10 +508,10 @@ static inline void audit_inode_child(const struct dentry *dentry,
}
void audit_core_dumps(long signr);
-static inline void audit_seccomp(unsigned long syscall)
+static inline void audit_seccomp(unsigned long syscall, long signr, int code)
{
if (unlikely(!audit_dummy_context()))
- __audit_seccomp(syscall);
+ __audit_seccomp(syscall, signr, code);
}
static inline void audit_ptrace(struct task_struct *t)
@@ -634,7 +634,7 @@ extern int audit_signals;
#define audit_inode(n,d) do { (void)(d); } while (0)
#define audit_inode_child(i,p) do { ; } while (0)
#define audit_core_dumps(i) do { ; } while (0)
-#define audit_seccomp(i) do { ; } while (0)
+#define audit_seccomp(i,s,c) do { ; } while (0)
#define auditsc_get_stamp(c,t,s) (0)
#define audit_get_loginuid(t) (-1)
#define audit_get_sessionid(t) (-1)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index af1de0f..4b96415 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -67,6 +67,7 @@
#include <linux/syscalls.h>
#include <linux/capability.h>
#include <linux/fs_struct.h>
+#include <linux/compat.h>
#include "audit.h"
@@ -2710,13 +2711,16 @@ void audit_core_dumps(long signr)
audit_log_end(ab);
}
-void __audit_seccomp(unsigned long syscall)
+void __audit_seccomp(unsigned long syscall, long signr, int code)
{
struct audit_buffer *ab;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_ANOM_ABEND);
- audit_log_abend(ab, "seccomp", SIGKILL);
+ audit_log_abend(ab, "seccomp", signr);
audit_log_format(ab, " syscall=%ld", syscall);
+ audit_log_format(ab, " compat=%d", is_compat_task());
+ audit_log_format(ab, " ip=0x%lx", KSTK_EIP(current));
+ audit_log_format(ab, " code=0x%x", code);
audit_log_end(ab);
}
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 0aeec19..0f7c709 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -60,18 +60,6 @@ struct seccomp_filter {
/* Limit any path through the tree to 256KB worth of instructions. */
#define MAX_INSNS_PER_PATH ((1 << 18) / sizeof(struct sock_filter))
-static void seccomp_filter_log_failure(int syscall)
-{
- int compat = 0;
-#ifdef CONFIG_COMPAT
- compat = is_compat_task();
-#endif
- pr_info("%s[%d]: %ssystem call %d blocked at 0x%lx\n",
- current->comm, task_pid_nr(current),
- (compat ? "compat " : ""),
- syscall, KSTK_EIP(current));
-}
-
/**
* get_u32 - returns a u32 offset into data
* @data: a unsigned 64 bit value
@@ -381,7 +369,6 @@ void __secure_computing(int this_syscall)
case SECCOMP_MODE_FILTER:
if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
return;
- seccomp_filter_log_failure(this_syscall);
exit_sig = SIGSYS;
break;
#endif
@@ -392,7 +379,7 @@ void __secure_computing(int this_syscall)
#ifdef SECCOMP_DEBUG
dump_stack();
#endif
- audit_seccomp(this_syscall);
+ audit_seccomp(this_syscall, exit_code, SECCOMP_RET_KILL);
do_exit(exit_sig);
}
--
1.7.5.4
^ permalink raw reply related
* [PATCH v18 10/15] seccomp: add SECCOMP_RET_ERRNO
From: Will Drewry @ 2012-04-12 21:47 UTC (permalink / raw)
To: linux-kernel
Cc: linux-man, linux-security-module, linux-arch, linux-doc,
kernel-hardening, netdev, x86, arnd, davem, hpa, mingo, oleg,
peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn, djm,
scarybeasts, indan, pmoore, akpm, corbet, eric.dumazet, markus,
coreyb, keescook, jmorris, Will Drewry
In-Reply-To: <1334267284-19166-1-git-send-email-wad@chromium.org>
This change adds the SECCOMP_RET_ERRNO as a valid return value from a
seccomp filter. Additionally, it makes the first use of the lower
16-bits for storing a filter-supplied errno. 16-bits is more than
enough for the errno-base.h calls.
Returning errors instead of immediately terminating processes that
violate seccomp policy allow for broader use of this functionality
for kernel attack surface reduction. For example, a linux container
could maintain a whitelist of pre-existing system calls but drop
all new ones with errnos. This would keep a logically static attack
surface while providing errnos that may allow for graceful failure
without the downside of do_exit() on a bad call.
This change also changes the signature of __secure_computing. It
appears the only direct caller is the arm entry code and it clobbers
any possible return value (register) immediately.
Signed-off-by: Will Drewry <wad@chromium.org>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Eric Paris <eparis@redhat.com>
v18: - fix up comments and rebase
- fix bad var name which was fixed in later revs
- remove _int() and just change the __secure_computing signature
v16-v17: ...
v15: - use audit_seccomp and add a skip label. (eparis@redhat.com)
- clean up and pad out return codes (indan@nul.nu)
v14: - no change/rebase
v13: - rebase on to 88ebdda6159ffc15699f204c33feb3e431bf9bdc
v12: - move to WARN_ON if filter is NULL
(oleg@redhat.com, luto@mit.edu, keescook@chromium.org)
- return immediately for filter==NULL (keescook@chromium.org)
- change evaluation to only compare the ACTION so that layered
errnos don't result in the lowest one being returned.
(keeschook@chromium.org)
v11: - check for NULL filter (keescook@chromium.org)
v10: - change loaders to fn
v9: - n/a
v8: - update Kconfig to note new need for syscall_set_return_value.
- reordered such that TRAP behavior follows on later.
- made the for loop a little less indent-y
v7: - introduced
---
arch/Kconfig | 6 ++++--
include/linux/seccomp.h | 10 ++++++----
kernel/seccomp.c | 42 ++++++++++++++++++++++++++++++++----------
3 files changed, 42 insertions(+), 16 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index 91c2c73..beaab68 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -220,8 +220,10 @@ config HAVE_ARCH_SECCOMP_FILTER
bool
help
This symbol should be selected by an architecure if it provides
- asm/syscall.h, specifically syscall_get_arguments() and
- syscall_get_arch().
+ asm/syscall.h, specifically syscall_get_arguments(),
+ syscall_get_arch(), and syscall_set_return_value(). Additionally,
+ its system call entry path must respect a return value of -1 from
+ __secure_computing() and/or secure_computing().
config SECCOMP_FILTER
def_bool y
diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 86bb68f..b4ce2c8 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -12,13 +12,14 @@
/*
* All BPF programs must return a 32-bit value.
- * The bottom 16-bits are reserved for future use.
+ * The bottom 16-bits are for optional return data.
* The upper 16-bits are ordered from least permissive values to most.
*
* The ordering ensures that a min_t() over composed return values always
* selects the least permissive choice.
*/
#define SECCOMP_RET_KILL 0x00000000U /* kill the task immediately */
+#define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */
#define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */
/* Masks for the return value sections. */
@@ -64,11 +65,12 @@ struct seccomp {
struct seccomp_filter *filter;
};
-extern void __secure_computing(int);
-static inline void secure_computing(int this_syscall)
+extern int __secure_computing(int);
+static inline int secure_computing(int this_syscall)
{
if (unlikely(test_thread_flag(TIF_SECCOMP)))
- __secure_computing(this_syscall);
+ return __secure_computing(this_syscall);
+ return 0;
}
extern long prctl_get_seccomp(void);
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 0f7c709..5f78fb6 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -199,15 +199,20 @@ static int seccomp_check_filter(struct sock_filter *filter, unsigned int flen)
static u32 seccomp_run_filters(int syscall)
{
struct seccomp_filter *f;
- u32 ret = SECCOMP_RET_KILL;
+ u32 ret = SECCOMP_RET_ALLOW;
+
+ /* Ensure unexpected behavior doesn't result in failing open. */
+ if (WARN_ON(current->seccomp.filter == NULL))
+ return SECCOMP_RET_KILL;
+
/*
* All filters in the list are evaluated and the lowest BPF return
- * value always takes priority.
+ * value always takes priority (ignoring the DATA).
*/
for (f = current->seccomp.filter; f; f = f->prev) {
- ret = sk_run_filter(NULL, f->insns);
- if (ret != SECCOMP_RET_ALLOW)
- break;
+ u32 cur_ret = sk_run_filter(NULL, f->insns);
+ if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
+ ret = cur_ret;
}
return ret;
}
@@ -346,11 +351,13 @@ static int mode1_syscalls_32[] = {
};
#endif
-void __secure_computing(int this_syscall)
+int __secure_computing(int this_syscall)
{
int mode = current->seccomp.mode;
int exit_sig = 0;
int *syscall;
+ u32 ret = SECCOMP_RET_KILL;
+ int data;
switch (mode) {
case SECCOMP_MODE_STRICT:
@@ -361,14 +368,26 @@ void __secure_computing(int this_syscall)
#endif
do {
if (*syscall == this_syscall)
- return;
+ return 0;
} while (*++syscall);
exit_sig = SIGKILL;
break;
#ifdef CONFIG_SECCOMP_FILTER
case SECCOMP_MODE_FILTER:
- if (seccomp_run_filters(this_syscall) == SECCOMP_RET_ALLOW)
- return;
+ ret = seccomp_run_filters(this_syscall);
+ data = ret & SECCOMP_RET_DATA;
+ switch (ret & SECCOMP_RET_ACTION) {
+ case SECCOMP_RET_ERRNO:
+ /* Set the low-order 16-bits as a errno. */
+ syscall_set_return_value(current, task_pt_regs(current),
+ -data, 0);
+ goto skip;
+ case SECCOMP_RET_ALLOW:
+ return 0;
+ case SECCOMP_RET_KILL:
+ default:
+ break;
+ }
exit_sig = SIGSYS;
break;
#endif
@@ -379,8 +398,11 @@ void __secure_computing(int this_syscall)
#ifdef SECCOMP_DEBUG
dump_stack();
#endif
- audit_seccomp(this_syscall, exit_code, SECCOMP_RET_KILL);
+ audit_seccomp(this_syscall, exit_sig, ret);
do_exit(exit_sig);
+skip:
+ audit_seccomp(this_syscall, exit_sig, ret);
+ return -1;
}
long prctl_get_seccomp(void)
--
1.7.5.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox