Re: [PATCH AUTOSEL 5.8 18/24] net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails

[Petko Manolov]

On 20-10-12 12:11:18, Joe Perches wrote:
> On Mon, 2020-10-12 at 15:02 -0400, Sasha Levin wrote:
> > From: Anant Thazhemadam 
> > 
> > [ Upstream commit f45a4248ea4cc13ed50618ff066849f9587226b2 ]
> > 
> > When get_registers() fails in set_ethernet_addr(),the uninitialized
> > value of node_id gets copied over as the address.
> > So, check the return value of get_registers().
> > 
> > If get_registers() executed successfully (i.e., it returns
> > sizeof(node_id)), copy over the MAC address using ether_addr_copy()
> > (instead of using memcpy()).
> > 
> > Else, if get_registers() failed instead, a randomly generated MAC
> > address is set as the MAC address instead.
> 
> This autosel is premature.
> 
> This patch always sets a random MAC.
> See the follow on patch: https://lkml.org/lkml/2020/10/11/131
> To my knowledge, this follow-ob has yet to be applied:

ACK, the follow-on patch has got the correct semantics.


Petko


> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> []
> > @@ -274,12 +274,20 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, 
> > __u8 indx, u16 reg)
> > return 1;
> >  }
> >  
> > -static inline void set_ethernet_addr(rtl8150_t * dev)
> > +static void set_ethernet_addr(rtl8150_t *dev)
> >  {
> > -   u8 node_id[6];
> > +   u8 node_id[ETH_ALEN];
> > +   int ret;
> > +
> > +   ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> >  
> > -   get_registers(dev, IDR, sizeof(node_id), node_id);
> > -   memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> > +   if (ret == sizeof(node_id)) {
> 
> So this needs to use
>   if (!ret) {
> 
> or 
>   if (ret < 0)
> 
> and reversed code blocks
> 
> > +   ether_addr_copy(dev->netdev->dev_addr, node_id);
> > +   } else {
> > +   eth_hw_addr_random(dev->netdev);
> > +   netdev_notice(dev->netdev, "Assigned a random MAC address: 
> > %pM\n",
> > + dev->netdev->dev_addr);
> > +   }
> >  }
> >  
> >  static int rtl8150_set_mac_address(struct net_device *netdev, void *p)
> 
> 

Re: [PATCH v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses

[Petko Manolov]

On 20-10-11 11:33:00, Joe Perches wrote:
> On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > > that was read must be copied over. Otherwise, a random ethernet address
> > > must be assigned.
> > > 
> > > get_registers() returns 0 if successful, and negative error number
> > > otherwise. However, in set_ethernet_addr(), this return value is
> > > incorrectly checked.
> > > 
> > > Since this return value will never be equal to sizeof(node_id), a
> > > random MAC address will always be generated and assigned to the
> > > device; even in cases when get_registers() is successful.
> > > 
> > > Correctly modifying the condition that checks if get_registers() was
> > > successful or not fixes this problem, and copies the ethernet address
> > > appropriately.
> 
> There are many unchecked uses of set_registers and get_registers
>  in this file.
> 
> If failures are really expected, then it might be better to fix
> them up too.

Checking the return value of each get/set_registers() is going to be a PITA and
not very helpful.  Doing so when setting the MAC address _does_ make sense as in
that case it is not a hard error.

In almost all other occasions if usb_control_msg_send/recv() return an error i'd
rather dump an error message (from within get/set_registers()) and let the user
decide whether to get rid of this adapter or start debugging it.


cheers,
Petko


> $ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c
> drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, 
> u16 size, void *data)
> drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, 
> u16 size, const void *data)
> drivers/net/usb/rtl8150.c:  set_registers(dev, PHYADD, sizeof(data), 
> data);
> drivers/net/usb/rtl8150.c:  set_registers(dev, PHYCNT, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:  get_registers(dev, PHYDAT, 2, data);
> drivers/net/usb/rtl8150.c:  set_registers(dev, PHYADD, sizeof(data), 
> data);
> drivers/net/usb/rtl8150.c:  set_registers(dev, PHYCNT, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:  ret = get_registers(dev, IDR, 
> sizeof(node_id), node_id);
> drivers/net/usb/rtl8150.c:  set_registers(dev, IDR, netdev->addr_len, 
> netdev->dev_addr);
> drivers/net/usb/rtl8150.c:  get_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, IDR_EEPROM + (i * 
> 2), 2,
> drivers/net/usb/rtl8150.c:  set_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, RCR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, TCR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, MSR, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, CR, 1, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, CSCR, 2, );
> drivers/net/usb/rtl8150.c:  set_registers(dev, IDR, 6, netdev->dev_addr);
> drivers/net/usb/rtl8150.c:  get_registers(dev, BMCR, 2, );
> drivers/net/usb/rtl8150.c:  get_registers(dev, ANLP, 2, );
> 
> 
> 

Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address

[Petko Manolov]

On 20-10-02 17:35:25, Anant Thazhemadam wrote:
> 
> Yes, this clears things up for me. I'll see to it that this gets done in a v3.

If set_ethernet_addr() fail, don't return error, but use eth_hw_addr_random() 
instead to set random MAC address and continue with the probing.

You can take a look here:
https://lore.kernel.org/netdev/20201002075604.44335-1-petko.mano...@konsulko.com/


cheers,
Petko

Re: [Linux-kernel-mentees][PATCH] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address

[Petko Manolov]

On 20-09-29 13:50:28, Anant Thazhemadam wrote:
> When get_registers() fails (which happens when usb_control_msg() fails)
> in set_ethernet_addr(), the uninitialized value of node_id gets copied
> as the address.
> 
> Checking for the return values appropriately, and handling the case
> wherein set_ethernet_addr() fails like this, helps in avoiding the
> mac address being incorrectly set in this manner.
> 
> Reported-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam 
> ---
>  drivers/net/usb/rtl8150.c | 24 
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 733f120c852b..e542a9ab2ff8 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -150,7 +150,7 @@ static const char driver_name [] = "rtl8150";
>  **   device related part of the code
>  **
>  */
> -static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> +static int get_registers(rtl8150_t *dev, u16 indx, u16 size, void *data)
>  {
>   void *buf;
>   int ret;
> @@ -274,12 +274,17 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 
> indx, u16 reg)
>   return 1;
>  }
>  
> -static inline void set_ethernet_addr(rtl8150_t * dev)
> +static bool set_ethernet_addr(rtl8150_t *dev)
>  {
>   u8 node_id[6];
> + int ret;
>  
> - get_registers(dev, IDR, sizeof(node_id), node_id);
> - memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> + ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> + if (ret > 0 && ret <= sizeof(node_id)) {

get_registers() was recently modified to use usb_control_msg_recv() which does
not return partial reads.  IOW you'll either get negative value or
sizeof(node_id).  Since it is good to be paranoid i'd convert the above check
to:

    if (ret == sizeof(node_id)) {

and fail in any other case.  Apart from this minor detail the rest of the patch 
looks good to me.

Acked-by: Petko Manolov

Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Petko Manolov]

On 20-09-24 13:09:05, Oliver Neukum wrote:
> Am Mittwoch, den 23.09.2020, 17:48 +0300 schrieb Petko Manolov:
> 
> > One possible fix is to add yet another argument to usb_control_msg_recv(), 
> > which would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of 
> > course.
> 
> submitted. The problem is those usages that are very hard to trace. I'd 
> dislike to just slab GFP_NOIO on them for no obvious reason.

Do you mean you submitted a patch for usb_control_msg_recv() (because i don't 
see it on linux-netdev) or i'm reading this all wrong?


Petko

Re: [PATCH 3/4] net: usb: rtl8150: use usb_control_msg_recv() and usb_control_msg_send()

[Petko Manolov]

On 20-09-23 12:22:37, Oliver Neukum wrote:
> Am Mittwoch, den 23.09.2020, 14:35 +0530 schrieb Himadri Pandya:
> 
> Hi,
> 
> > Many usage of usb_control_msg() do not have proper error check on return
> > value leaving scope for bugs on short reads. New usb_control_msg_recv()
> > and usb_control_msg_send() nicely wraps usb_control_msg() with proper
> > error check. Hence use the wrappers instead of calling usb_control_msg()
> > directly.
> > 
> > Signed-off-by: Himadri Pandya 
> Nacked-by: Oliver Neukum 
> 
> > ---
> >  drivers/net/usb/rtl8150.c | 32 ++--
> >  1 file changed, 6 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index 733f120c852b..e3002b675921 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -152,36 +152,16 @@ static const char driver_name [] = "rtl8150";
> >  */
> >  static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> >  {
> > -   void *buf;
> > -   int ret;
> > -
> > -   buf = kmalloc(size, GFP_NOIO);
> 
> GFP_NOIO is used here for a reason. You need to use this helper
> while in contexts of error recovery and runtime PM.
> 
> > -   if (!buf)
> > -   return -ENOMEM;
> > -
> > -   ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > - RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > - indx, 0, buf, size, 500);
> > -   if (ret > 0 && ret <= size)
> > -   memcpy(data, buf, ret);
> > -   kfree(buf);
> > -   return ret;
> > +   return usb_control_msg_recv(dev->udev, 0, RTL8150_REQ_GET_REGS,
> > +   RTL8150_REQT_READ, indx, 0, data,
> > +   size, 500);
> 
> This internally uses kmemdup() with GFP_KERNEL.
> You cannot make this change. The API does not support it.
> I am afraid we will have to change the API first, before more
> such changes are done.

One possible fix is to add yet another argument to usb_control_msg_recv(), 
which 
would be the GFP_XYZ flag to pass on to kmemdup().  Up to Greg, of course.


cheers,
Petko

Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs on failed register reads

[Petko Manolov]

On 20-09-16 10:35:40, Anant Thazhemadam wrote:
> get_registers() copies whatever memory is written by the
> usb_control_msg() call even if the underlying urb call ends up failing.

Not true, memcpy() is only called if "ret" is positive.

> If get_registers() fails, or ends up reading 0 bytes, meaningless and junk 
> register values would end up being copied over (and eventually read by the 
> driver), and since most of the callers of get_registers() don't check the 
> return values of get_registers() either, this would go unnoticed.

usb_control_msg() returns negative on error (look up usb_internal_control_msg() 
to see for yourself) so it does not go unnoticed.  If for some reason it return 
zero, nothing is copied.  Also, if usb transfer fail no register values are 
being copied anywhere.

Your patch also allows for memcpy() to be called with 'size' either zero or 
greater than the allocated buffer size. Please, look at the code carefully.

> It might be a better idea to try and mirror the PCI master abort
> termination and set memory to 0xFFs instead in such cases.

I wasn't aware drivers are now responsible for filling up the memory with 
anything.  Does not sound like a good idea to me.

> Fixes: https://syzkaller.appspot.com/bug?extid=abbc768b560c84d92fd3
> Reported-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> Signed-off-by: Anant Thazhemadam 

Well, NACK from me.


cheers,
Petko


> ---
>  drivers/net/usb/rtl8150.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 733f120c852b..04fca7bfcbcb 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -162,8 +162,13 @@ static int get_registers(rtl8150_t * dev, u16 indx, u16 
> size, void *data)
>   ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> indx, 0, buf, size, 500);
> - if (ret > 0 && ret <= size)
> +
> + if (ret < 0)
> + memset(data, 0xff, size);
> +
> + else
>   memcpy(data, buf, ret);
> +
>   kfree(buf);
>   return ret;
>  }
> @@ -276,7 +281,7 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 
> indx, u16 reg)
>  
>  static inline void set_ethernet_addr(rtl8150_t * dev)
>  {
> - u8 node_id[6];
> + u8 node_id[6] = {0};
>  
>   get_registers(dev, IDR, sizeof(node_id), node_id);
>   memcpy(dev->netdev->dev_addr, node_id, sizeof(node_id));
> -- 
> 2.25.1
> 
> 

Re: [Linux-kernel-mentees][PATCH] rtl8150: set memory to all 0xFFs on failed register reads

[Petko Manolov]

On 20-09-16 08:22:27, Greg KH wrote:
> On Wed, Sep 16, 2020 at 10:35:40AM +0530, Anant Thazhemadam wrote:
> > get_registers() copies whatever memory is written by the
> > usb_control_msg() call even if the underlying urb call ends up failing.
> > 
> > If get_registers() fails, or ends up reading 0 bytes, meaningless and 
> > junk register values would end up being copied over (and eventually read 
> > by the driver), and since most of the callers of get_registers() don't 
> > check the return values of get_registers() either, this would go unnoticed.
> > 
> > It might be a better idea to try and mirror the PCI master abort
> > termination and set memory to 0xFFs instead in such cases.
> 
> It would be better to use this new api call instead of
> usb_control_msg():
>   
> https://lore.kernel.org/r/20200914153756.3412156-1-gre...@linuxfoundation.org

Heh, wasn't aware of the new api.

> How about porting this patch to run on top of that series instead?  That 
> should make this logic much simpler.

I'll need to check if in this case 'size' is the right amount of bytes expected 
and not an upper limit.  Then i'll convert it to the new api.


cheers,
Petko


> > Fixes: https://syzkaller.appspot.com/bug?extid=abbc768b560c84d92fd3
> > Reported-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> > Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com
> > Signed-off-by: Anant Thazhemadam 
> > ---
> >  drivers/net/usb/rtl8150.c | 9 +++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index 733f120c852b..04fca7bfcbcb 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -162,8 +162,13 @@ static int get_registers(rtl8150_t * dev, u16 indx, 
> > u16 size, void *data)
> > ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> >   RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> >   indx, 0, buf, size, 500);
> > -   if (ret > 0 && ret <= size)
> > +
> > +   if (ret < 0)
> > +   memset(data, 0xff, size);
> > +
> > +   else
> > memcpy(data, buf, ret);
> > +
> > kfree(buf);
> > return ret;
> >  }
> > @@ -276,7 +281,7 @@ static int write_mii_word(rtl8150_t * dev, u8 phy, __u8 
> > indx, u16 reg)
> >  
> >  static inline void set_ethernet_addr(rtl8150_t * dev)
> >  {
> > -   u8 node_id[6];
> > +   u8 node_id[6] = {0};
> 
> This should not be needed to be done.
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH] net: usb: pegasus: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-17 23:34:04, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

Yep, the patch seems to be working fine on real hardware.

Acked-by: Petko Manolov <pet...@nucleusys.com>


cheers,
Petko


> Signed-off-by: Philippe Reynes <trem...@gmail.com>
> ---
>  drivers/net/usb/pegasus.c |   14 --
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 3667448..321e059 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -953,20 +953,22 @@ static inline void pegasus_reset_wol(struct net_device 
> *dev)
>  }
>  
>  static int
> -pegasus_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_get_link_ksettings(struct net_device *dev,
> +struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus;
>  
>   pegasus = netdev_priv(dev);
> - mii_ethtool_gset(>mii, ecmd);
> + mii_ethtool_get_link_ksettings(>mii, ecmd);
>   return 0;
>  }
>  
>  static int
> -pegasus_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_set_link_ksettings(struct net_device *dev,
> +const struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus = netdev_priv(dev);
> - return mii_ethtool_sset(>mii, ecmd);
> + return mii_ethtool_set_link_ksettings(>mii, ecmd);
>  }
>  
>  static int pegasus_nway_reset(struct net_device *dev)
> @@ -995,14 +997,14 @@ static void pegasus_set_msglevel(struct net_device 
> *dev, u32 v)
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = pegasus_get_drvinfo,
> - .get_settings = pegasus_get_settings,
> - .set_settings = pegasus_set_settings,
>   .nway_reset = pegasus_nway_reset,
>   .get_link = pegasus_get_link,
>   .get_msglevel = pegasus_get_msglevel,
>   .set_msglevel = pegasus_set_msglevel,
>   .get_wol = pegasus_get_wol,
>   .set_wol = pegasus_set_wol,
> + .get_link_ksettings = pegasus_get_link_ksettings,
> + .set_link_ksettings = pegasus_set_link_ksettings,
>  };
>  
>  static int pegasus_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH] net: usb: pegasus: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-17 23:34:04, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

Yep, the patch seems to be working fine on real hardware.

Acked-by: Petko Manolov 


cheers,
Petko


> Signed-off-by: Philippe Reynes 
> ---
>  drivers/net/usb/pegasus.c |   14 --
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 3667448..321e059 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -953,20 +953,22 @@ static inline void pegasus_reset_wol(struct net_device 
> *dev)
>  }
>  
>  static int
> -pegasus_get_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_get_link_ksettings(struct net_device *dev,
> +struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus;
>  
>   pegasus = netdev_priv(dev);
> - mii_ethtool_gset(>mii, ecmd);
> + mii_ethtool_get_link_ksettings(>mii, ecmd);
>   return 0;
>  }
>  
>  static int
> -pegasus_set_settings(struct net_device *dev, struct ethtool_cmd *ecmd)
> +pegasus_set_link_ksettings(struct net_device *dev,
> +const struct ethtool_link_ksettings *ecmd)
>  {
>   pegasus_t *pegasus = netdev_priv(dev);
> - return mii_ethtool_sset(>mii, ecmd);
> + return mii_ethtool_set_link_ksettings(>mii, ecmd);
>  }
>  
>  static int pegasus_nway_reset(struct net_device *dev)
> @@ -995,14 +997,14 @@ static void pegasus_set_msglevel(struct net_device 
> *dev, u32 v)
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = pegasus_get_drvinfo,
> - .get_settings = pegasus_get_settings,
> - .set_settings = pegasus_set_settings,
>   .nway_reset = pegasus_nway_reset,
>   .get_link = pegasus_get_link,
>   .get_msglevel = pegasus_get_msglevel,
>   .set_msglevel = pegasus_set_msglevel,
>   .get_wol = pegasus_get_wol,
>   .set_wol = pegasus_set_wol,
> + .get_link_ksettings = pegasus_get_link_ksettings,
> + .set_link_ksettings = pegasus_set_link_ksettings,
>  };
>  
>  static int pegasus_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-13 17:00:20, Petko Manolov wrote:
> On 17-03-12 23:16:25, Philippe Reynes wrote:
> > The ethtool api {get|set}_settings is deprecated. We move this driver to 
> > new 
> > api {get|set}_link_ksettings.
> > 
> > As I don't have the hardware, I'd be very pleased if someone may test this 
> > patch.
> 
> I've got some old adapters around and will drop you a line when i test the 
> patch.

The adapter is working fine with your patch.  You may add:

Acked-by: Petko Manolov <pet...@nucleusys.com>


cheers,
Petko


> > Signed-off-by: Philippe Reynes <trem...@gmail.com>
> > ---
> >  drivers/net/usb/rtl8150.c |   35 ---
> >  1 files changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index c81c791..daaa88a 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> > *netdev, struct ethtool_drvinf
> > usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
> >  }
> >  
> > -static int rtl8150_get_settings(struct net_device *netdev, struct 
> > ethtool_cmd *ecmd)
> > +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> > + struct ethtool_link_ksettings *ecmd)
> >  {
> > rtl8150_t *dev = netdev_priv(netdev);
> > short lpa, bmcr;
> > +   u32 supported;
> >  
> > -   ecmd->supported = (SUPPORTED_10baseT_Half |
> > +   supported = (SUPPORTED_10baseT_Half |
> >   SUPPORTED_10baseT_Full |
> >   SUPPORTED_100baseT_Half |
> >   SUPPORTED_100baseT_Full |
> >   SUPPORTED_Autoneg |
> >   SUPPORTED_TP | SUPPORTED_MII);
> > -   ecmd->port = PORT_TP;
> > -   ecmd->transceiver = XCVR_INTERNAL;
> > -   ecmd->phy_address = dev->phy;
> > +   ecmd->base.port = PORT_TP;
> > +   ecmd->base.phy_address = dev->phy;
> > get_registers(dev, BMCR, 2, );
> > get_registers(dev, ANLP, 2, );
> > if (bmcr & BMCR_ANENABLE) {
> > u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
> >  SPEED_100 : SPEED_10);
> > -   ethtool_cmd_speed_set(ecmd, speed);
> > -   ecmd->autoneg = AUTONEG_ENABLE;
> > +   ecmd->base.speed = speed;
> > +   ecmd->base.autoneg = AUTONEG_ENABLE;
> > if (speed == SPEED_100)
> > -   ecmd->duplex = (lpa & LPA_100FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_100FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > else
> > -   ecmd->duplex = (lpa & LPA_10FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_10FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > } else {
> > -   ecmd->autoneg = AUTONEG_DISABLE;
> > -   ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> > -SPEED_100 : SPEED_10));
> > -   ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> > +   ecmd->base.autoneg = AUTONEG_DISABLE;
> > +   ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> > +SPEED_100 : SPEED_10);
> > +   ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > }
> > +
> > +   ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> > +   supported);
> > +
> > return 0;
> >  }
> >  
> >  static const struct ethtool_ops ops = {
> > .get_drvinfo = rtl8150_get_drvinfo,
> > -   .get_settings = rtl8150_get_settings,
> > -   .get_link = ethtool_op_get_link
> > +   .get_link = ethtool_op_get_link,
> > +   .get_link_ksettings = rtl8150_get_link_ksettings,
> >  };
> >  
> >  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> > cmd)
> > -- 
> > 1.7.4.4
> > 
> > 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-13 17:00:20, Petko Manolov wrote:
> On 17-03-12 23:16:25, Philippe Reynes wrote:
> > The ethtool api {get|set}_settings is deprecated. We move this driver to 
> > new 
> > api {get|set}_link_ksettings.
> > 
> > As I don't have the hardware, I'd be very pleased if someone may test this 
> > patch.
> 
> I've got some old adapters around and will drop you a line when i test the 
> patch.

The adapter is working fine with your patch.  You may add:

Acked-by: Petko Manolov 


cheers,
Petko


> > Signed-off-by: Philippe Reynes 
> > ---
> >  drivers/net/usb/rtl8150.c |   35 ---
> >  1 files changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index c81c791..daaa88a 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> > *netdev, struct ethtool_drvinf
> > usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
> >  }
> >  
> > -static int rtl8150_get_settings(struct net_device *netdev, struct 
> > ethtool_cmd *ecmd)
> > +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> > + struct ethtool_link_ksettings *ecmd)
> >  {
> > rtl8150_t *dev = netdev_priv(netdev);
> > short lpa, bmcr;
> > +   u32 supported;
> >  
> > -   ecmd->supported = (SUPPORTED_10baseT_Half |
> > +   supported = (SUPPORTED_10baseT_Half |
> >   SUPPORTED_10baseT_Full |
> >   SUPPORTED_100baseT_Half |
> >   SUPPORTED_100baseT_Full |
> >   SUPPORTED_Autoneg |
> >   SUPPORTED_TP | SUPPORTED_MII);
> > -   ecmd->port = PORT_TP;
> > -   ecmd->transceiver = XCVR_INTERNAL;
> > -   ecmd->phy_address = dev->phy;
> > +   ecmd->base.port = PORT_TP;
> > +   ecmd->base.phy_address = dev->phy;
> > get_registers(dev, BMCR, 2, );
> > get_registers(dev, ANLP, 2, );
> > if (bmcr & BMCR_ANENABLE) {
> > u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
> >  SPEED_100 : SPEED_10);
> > -   ethtool_cmd_speed_set(ecmd, speed);
> > -   ecmd->autoneg = AUTONEG_ENABLE;
> > +   ecmd->base.speed = speed;
> > +   ecmd->base.autoneg = AUTONEG_ENABLE;
> > if (speed == SPEED_100)
> > -   ecmd->duplex = (lpa & LPA_100FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_100FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > else
> > -   ecmd->duplex = (lpa & LPA_10FULL) ?
> > +   ecmd->base.duplex = (lpa & LPA_10FULL) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > } else {
> > -   ecmd->autoneg = AUTONEG_DISABLE;
> > -   ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> > -SPEED_100 : SPEED_10));
> > -   ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> > +   ecmd->base.autoneg = AUTONEG_DISABLE;
> > +   ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> > +SPEED_100 : SPEED_10);
> > +   ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
> > DUPLEX_FULL : DUPLEX_HALF;
> > }
> > +
> > +   ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> > +   supported);
> > +
> > return 0;
> >  }
> >  
> >  static const struct ethtool_ops ops = {
> > .get_drvinfo = rtl8150_get_drvinfo,
> > -   .get_settings = rtl8150_get_settings,
> > -   .get_link = ethtool_op_get_link
> > +   .get_link = ethtool_op_get_link,
> > +   .get_link_ksettings = rtl8150_get_link_ksettings,
> >  };
> >  
> >  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> > cmd)
> > -- 
> > 1.7.4.4
> > 
> > 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-12 23:16:25, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

I've got some old adapters around and will drop you a line when i test the 
patch.


Petko


> Signed-off-by: Philippe Reynes 
> ---
>  drivers/net/usb/rtl8150.c |   35 ---
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index c81c791..daaa88a 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> *netdev, struct ethtool_drvinf
>   usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
>  }
>  
> -static int rtl8150_get_settings(struct net_device *netdev, struct 
> ethtool_cmd *ecmd)
> +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> +   struct ethtool_link_ksettings *ecmd)
>  {
>   rtl8150_t *dev = netdev_priv(netdev);
>   short lpa, bmcr;
> + u32 supported;
>  
> - ecmd->supported = (SUPPORTED_10baseT_Half |
> + supported = (SUPPORTED_10baseT_Half |
> SUPPORTED_10baseT_Full |
> SUPPORTED_100baseT_Half |
> SUPPORTED_100baseT_Full |
> SUPPORTED_Autoneg |
> SUPPORTED_TP | SUPPORTED_MII);
> - ecmd->port = PORT_TP;
> - ecmd->transceiver = XCVR_INTERNAL;
> - ecmd->phy_address = dev->phy;
> + ecmd->base.port = PORT_TP;
> + ecmd->base.phy_address = dev->phy;
>   get_registers(dev, BMCR, 2, );
>   get_registers(dev, ANLP, 2, );
>   if (bmcr & BMCR_ANENABLE) {
>   u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
>SPEED_100 : SPEED_10);
> - ethtool_cmd_speed_set(ecmd, speed);
> - ecmd->autoneg = AUTONEG_ENABLE;
> + ecmd->base.speed = speed;
> + ecmd->base.autoneg = AUTONEG_ENABLE;
>   if (speed == SPEED_100)
> - ecmd->duplex = (lpa & LPA_100FULL) ?
> + ecmd->base.duplex = (lpa & LPA_100FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   else
> - ecmd->duplex = (lpa & LPA_10FULL) ?
> + ecmd->base.duplex = (lpa & LPA_10FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   } else {
> - ecmd->autoneg = AUTONEG_DISABLE;
> - ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> -  SPEED_100 : SPEED_10));
> - ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> + ecmd->base.autoneg = AUTONEG_DISABLE;
> + ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> +  SPEED_100 : SPEED_10);
> + ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   }
> +
> + ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> + supported);
> +
>   return 0;
>  }
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = rtl8150_get_drvinfo,
> - .get_settings = rtl8150_get_settings,
> - .get_link = ethtool_op_get_link
> + .get_link = ethtool_op_get_link,
> + .get_link_ksettings = rtl8150_get_link_ksettings,
>  };
>  
>  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH] net: usb: rtl8150: use new api ethtool_{get|set}_link_ksettings

[Petko Manolov]

On 17-03-12 23:16:25, Philippe Reynes wrote:
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
> As I don't have the hardware, I'd be very pleased if someone may test this 
> patch.

I've got some old adapters around and will drop you a line when i test the 
patch.


Petko


> Signed-off-by: Philippe Reynes 
> ---
>  drivers/net/usb/rtl8150.c |   35 ---
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index c81c791..daaa88a 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -791,47 +791,52 @@ static void rtl8150_get_drvinfo(struct net_device 
> *netdev, struct ethtool_drvinf
>   usb_make_path(dev->udev, info->bus_info, sizeof(info->bus_info));
>  }
>  
> -static int rtl8150_get_settings(struct net_device *netdev, struct 
> ethtool_cmd *ecmd)
> +static int rtl8150_get_link_ksettings(struct net_device *netdev,
> +   struct ethtool_link_ksettings *ecmd)
>  {
>   rtl8150_t *dev = netdev_priv(netdev);
>   short lpa, bmcr;
> + u32 supported;
>  
> - ecmd->supported = (SUPPORTED_10baseT_Half |
> + supported = (SUPPORTED_10baseT_Half |
> SUPPORTED_10baseT_Full |
> SUPPORTED_100baseT_Half |
> SUPPORTED_100baseT_Full |
> SUPPORTED_Autoneg |
> SUPPORTED_TP | SUPPORTED_MII);
> - ecmd->port = PORT_TP;
> - ecmd->transceiver = XCVR_INTERNAL;
> - ecmd->phy_address = dev->phy;
> + ecmd->base.port = PORT_TP;
> + ecmd->base.phy_address = dev->phy;
>   get_registers(dev, BMCR, 2, );
>   get_registers(dev, ANLP, 2, );
>   if (bmcr & BMCR_ANENABLE) {
>   u32 speed = ((lpa & (LPA_100HALF | LPA_100FULL)) ?
>SPEED_100 : SPEED_10);
> - ethtool_cmd_speed_set(ecmd, speed);
> - ecmd->autoneg = AUTONEG_ENABLE;
> + ecmd->base.speed = speed;
> + ecmd->base.autoneg = AUTONEG_ENABLE;
>   if (speed == SPEED_100)
> - ecmd->duplex = (lpa & LPA_100FULL) ?
> + ecmd->base.duplex = (lpa & LPA_100FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   else
> - ecmd->duplex = (lpa & LPA_10FULL) ?
> + ecmd->base.duplex = (lpa & LPA_10FULL) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   } else {
> - ecmd->autoneg = AUTONEG_DISABLE;
> - ethtool_cmd_speed_set(ecmd, ((bmcr & BMCR_SPEED100) ?
> -  SPEED_100 : SPEED_10));
> - ecmd->duplex = (bmcr & BMCR_FULLDPLX) ?
> + ecmd->base.autoneg = AUTONEG_DISABLE;
> + ecmd->base.speed = ((bmcr & BMCR_SPEED100) ?
> +  SPEED_100 : SPEED_10);
> + ecmd->base.duplex = (bmcr & BMCR_FULLDPLX) ?
>   DUPLEX_FULL : DUPLEX_HALF;
>   }
> +
> + ethtool_convert_legacy_u32_to_link_mode(ecmd->link_modes.supported,
> + supported);
> +
>   return 0;
>  }
>  
>  static const struct ethtool_ops ops = {
>   .get_drvinfo = rtl8150_get_drvinfo,
> - .get_settings = rtl8150_get_settings,
> - .get_link = ethtool_op_get_link
> + .get_link = ethtool_op_get_link,
> + .get_link_ksettings = rtl8150_get_link_ksettings,
>  };
>  
>  static int rtl8150_ioctl(struct net_device *netdev, struct ifreq *rq, int 
> cmd)
> -- 
> 1.7.4.4
> 
> 

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-17 09:56:00, David Howells wrote:
> Petko Manolov <pet...@mip-labs.com> wrote:
> 
> > On 16-11-16 18:11:13, David Howells wrote:
> > > Allow keys to be added to the system secondary certificates keyring 
> > > during 
> > > kernel initialisation in an unrestricted fashion.  Such keys are 
> > > implicitly trusted and don't have their trust chains checked on link.
> > 
> > Well, I for one do not explicitly trust these keys.  I may even want to 
> > completely remove or replace them.
> 
> Fine be me.  However, if you remove them all I would guess that you cannot 
> perform a secure boot.

Maybe not on PC, but there's plenty of other architectures out there.  What if 
i 
replace all UEFI keys with my own?

> Note that it's to be expected that the keys being loaded from the UEFI 
> database cannot have their signatures checked - which is why they would have 
> to be implicitly trusted.  For the same reason, the kernel does not check the 
> signatures on the keys compiled into the kernel image.

I build all kernels that matter to me and i _do_ trust myself.  Unfortunately i 
can't say the same for any corporation out there.

Trusting a key because your vendor shipped the HW with it so that you have no 
way to verify the signature is pretty weak argument IMHO.

However, I am also well aware that most people just don't care. :)

> > > This allows keys in the UEFI database to be added in secure boot mode for 
> > > the purposes of module signing.
> > 
> > The key import should not be automatic, it should be optional.
> 
> You can argue this either way.  There's a config option to allow you to turn 
> this on or off.  Arguably, this should be split in two: one for the whitelist 
> (db, MokListRT) and one for the blacklist (dbx).

I did not see the config option.  There is one?

Right now i can't decide whether whitelist should go along with blacklist or 
there should be separate options.  I guess for whoever goes down this path it 
would make sense to use either both or none of them.

> Further, possibly I should add an option that allows this to be restricted to
> secure boot mode only.

Please do.  It doesn't make much sense otherwise.

> > Same applies to the validation process.
> 
> Depends what you mean by "the validation process"?  The use of secure boot at 
> all?  The checking of signatures on keys?  Module signing?

Nevermind.  The keys signature can't be verified in the classic UEFI case.


Petko

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-17 09:56:00, David Howells wrote:
> Petko Manolov  wrote:
> 
> > On 16-11-16 18:11:13, David Howells wrote:
> > > Allow keys to be added to the system secondary certificates keyring 
> > > during 
> > > kernel initialisation in an unrestricted fashion.  Such keys are 
> > > implicitly trusted and don't have their trust chains checked on link.
> > 
> > Well, I for one do not explicitly trust these keys.  I may even want to 
> > completely remove or replace them.
> 
> Fine be me.  However, if you remove them all I would guess that you cannot 
> perform a secure boot.

Maybe not on PC, but there's plenty of other architectures out there.  What if 
i 
replace all UEFI keys with my own?

> Note that it's to be expected that the keys being loaded from the UEFI 
> database cannot have their signatures checked - which is why they would have 
> to be implicitly trusted.  For the same reason, the kernel does not check the 
> signatures on the keys compiled into the kernel image.

I build all kernels that matter to me and i _do_ trust myself.  Unfortunately i 
can't say the same for any corporation out there.

Trusting a key because your vendor shipped the HW with it so that you have no 
way to verify the signature is pretty weak argument IMHO.

However, I am also well aware that most people just don't care. :)

> > > This allows keys in the UEFI database to be added in secure boot mode for 
> > > the purposes of module signing.
> > 
> > The key import should not be automatic, it should be optional.
> 
> You can argue this either way.  There's a config option to allow you to turn 
> this on or off.  Arguably, this should be split in two: one for the whitelist 
> (db, MokListRT) and one for the blacklist (dbx).

I did not see the config option.  There is one?

Right now i can't decide whether whitelist should go along with blacklist or 
there should be separate options.  I guess for whoever goes down this path it 
would make sense to use either both or none of them.

> Further, possibly I should add an option that allows this to be restricted to
> secure boot mode only.

Please do.  It doesn't make much sense otherwise.

> > Same applies to the validation process.
> 
> Depends what you mean by "the validation process"?  The use of secure boot at 
> all?  The checking of signatures on keys?  Module signing?

Nevermind.  The keys signature can't be verified in the classic UEFI case.


Petko

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-16 18:11:13, David Howells wrote:
> Allow keys to be added to the system secondary certificates keyring during 
> kernel initialisation in an unrestricted fashion.  Such keys are implicitly 
> trusted and don't have their trust chains checked on link.

Well, I for one do not explicitly trust these keys.  I may even want to 
completely remove or replace them.

> This allows keys in the UEFI database to be added in secure boot mode for the 
> purposes of module signing.

The key import should not be automatic, it should be optional.  Same applies to 
the validation process.


Petko


> Signed-off-by: David Howells 
> ---
> 
>  certs/internal.h   |   18 ++
>  certs/system_keyring.c |   33 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 certs/internal.h
> 
> diff --git a/certs/internal.h b/certs/internal.h
> new file mode 100644
> index ..5dcbefb0c23a
> --- /dev/null
> +++ b/certs/internal.h
> @@ -0,0 +1,18 @@
> +/* Internal definitions
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowe...@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +/*
> + * system_keyring.c
> + */
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +extern void __init add_trusted_secondary_key(const char *source,
> +  const void *data, size_t len);
> +#endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 50979d6dcecd..dfddcf6e6c88 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include "internal.h"
>  
>  static struct key *builtin_trusted_keys;
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> @@ -242,3 +243,35 @@ int verify_pkcs7_signature(const void *data, size_t len,
>  EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
>  
>  #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +/**
> + * add_trusted_secondary_key - Add to secondary keyring with no validation
> + * @source: Source of key
> + * @data: The blob holding the key
> + * @len: The length of the data blob
> + *
> + * Add a key to the secondary keyring without checking its trust chain.  This
> + * is available only during kernel initialisation.
> + */
> +void __init add_trusted_secondary_key(const char *source,
> +   const void *data, size_t len)
> +{
> + key_ref_t key;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len,
> +(KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +KEY_USR_VIEW,
> +KEY_ALLOC_NOT_IN_QUOTA |
> +KEY_ALLOC_BYPASS_RESTRICTION);
> +
> + if (IS_ERR(key))
> + pr_err("Problem loading %s X.509 certificate (%ld)\n",
> +source, PTR_ERR(key));
> + else
> + pr_notice("Loaded %s cert '%s' linked to secondary sys 
> keyring\n",
> +   source, key_ref_to_ptr(key)->description);
> +}
> +#endif /* CONFIG_SECONDARY_TRUSTED_KEYRING */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-16 18:11:13, David Howells wrote:
> Allow keys to be added to the system secondary certificates keyring during 
> kernel initialisation in an unrestricted fashion.  Such keys are implicitly 
> trusted and don't have their trust chains checked on link.

Well, I for one do not explicitly trust these keys.  I may even want to 
completely remove or replace them.

> This allows keys in the UEFI database to be added in secure boot mode for the 
> purposes of module signing.

The key import should not be automatic, it should be optional.  Same applies to 
the validation process.


Petko


> Signed-off-by: David Howells 
> ---
> 
>  certs/internal.h   |   18 ++
>  certs/system_keyring.c |   33 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 certs/internal.h
> 
> diff --git a/certs/internal.h b/certs/internal.h
> new file mode 100644
> index ..5dcbefb0c23a
> --- /dev/null
> +++ b/certs/internal.h
> @@ -0,0 +1,18 @@
> +/* Internal definitions
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowe...@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +/*
> + * system_keyring.c
> + */
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +extern void __init add_trusted_secondary_key(const char *source,
> +  const void *data, size_t len);
> +#endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 50979d6dcecd..dfddcf6e6c88 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include "internal.h"
>  
>  static struct key *builtin_trusted_keys;
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> @@ -242,3 +243,35 @@ int verify_pkcs7_signature(const void *data, size_t len,
>  EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
>  
>  #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +/**
> + * add_trusted_secondary_key - Add to secondary keyring with no validation
> + * @source: Source of key
> + * @data: The blob holding the key
> + * @len: The length of the data blob
> + *
> + * Add a key to the secondary keyring without checking its trust chain.  This
> + * is available only during kernel initialisation.
> + */
> +void __init add_trusted_secondary_key(const char *source,
> +   const void *data, size_t len)
> +{
> + key_ref_t key;
> +
> + key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +"asymmetric",
> +NULL, data, len,
> +(KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +KEY_USR_VIEW,
> +KEY_ALLOC_NOT_IN_QUOTA |
> +KEY_ALLOC_BYPASS_RESTRICTION);
> +
> + if (IS_ERR(key))
> + pr_err("Problem loading %s X.509 certificate (%ld)\n",
> +source, PTR_ERR(key));
> + else
> + pr_notice("Loaded %s cert '%s' linked to secondary sys 
> keyring\n",
> +   source, key_ref_to_ptr(key)->description);
> +}
> +#endif /* CONFIG_SECONDARY_TRUSTED_KEYRING */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On August 31, 2016 5:23:05 PM GMT+03:00, Tejun Heo <t...@kernel.org> wrote:
>On Tue, Aug 30, 2016 at 10:02:47PM +0530, Bhaktipriya Shridhar wrote:
>> The workqueue "pegasus_workqueue" queues a single work item per
>pegasus
>> instance and hence it doesn't require execution ordering. Hence,
>> alloc_workqueue has been used to replace the deprecated
>> create_singlethread_workqueue instance.
>> 
>> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
>> memory pressure since it's a network driver.
>> 
>> Since there are fixed number of work items, explicit concurrency
>> limit is unnecessary here.
>> 
>> Signed-off-by: Bhaktipriya Shridhar <bhaktipriy...@gmail.com>
>
>Acked-by: Tejun Heo <t...@kernel.org>
>
>Thanks.


Acked-by: Petko Manolov <pet...@mip-labs.com>


cheers,
Petko

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On August 31, 2016 5:23:05 PM GMT+03:00, Tejun Heo  wrote:
>On Tue, Aug 30, 2016 at 10:02:47PM +0530, Bhaktipriya Shridhar wrote:
>> The workqueue "pegasus_workqueue" queues a single work item per
>pegasus
>> instance and hence it doesn't require execution ordering. Hence,
>> alloc_workqueue has been used to replace the deprecated
>> create_singlethread_workqueue instance.
>> 
>> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
>> memory pressure since it's a network driver.
>> 
>> Since there are fixed number of work items, explicit concurrency
>> limit is unnecessary here.
>> 
>> Signed-off-by: Bhaktipriya Shridhar 
>
>Acked-by: Tejun Heo 
>
>Thanks.


Acked-by: Petko Manolov 


cheers,
Petko

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On 16-08-30 22:02:47, Bhaktipriya Shridhar wrote:
> The workqueue "pegasus_workqueue" queues a single work item per pegasus
> instance and hence it doesn't require execution ordering. Hence,
> alloc_workqueue has been used to replace the deprecated
> create_singlethread_workqueue instance.
> 
> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
> memory pressure since it's a network driver.
> 
> Since there are fixed number of work items, explicit concurrency
> limit is unnecessary here.
> 
> Signed-off-by: Bhaktipriya Shridhar 
> ---
>  drivers/net/usb/pegasus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9bbe0161..1434e5d 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -1129,7 +1129,8 @@ static int pegasus_probe(struct usb_interface *intf,
>   return -ENODEV;
> 
>   if (pegasus_count == 0) {
> - pegasus_workqueue = create_singlethread_workqueue("pegasus");
> + pegasus_workqueue = alloc_workqueue("pegasus", WQ_MEM_RECLAIM,
> + 0);
>   if (!pegasus_workqueue)
>   return -ENOMEM;
>   }
> --
> 2.1.4

Nope, there is no need for singlethread-ness here.  As long as the flag you 
used 
is doing the right thing i am OK with the patch.


Petko

Re: [PATCH] net: pegasus: Remove deprecated create_singlethread_workqueue

[Petko Manolov]

On 16-08-30 22:02:47, Bhaktipriya Shridhar wrote:
> The workqueue "pegasus_workqueue" queues a single work item per pegasus
> instance and hence it doesn't require execution ordering. Hence,
> alloc_workqueue has been used to replace the deprecated
> create_singlethread_workqueue instance.
> 
> The WQ_MEM_RECLAIM flag has been set to ensure forward progress under
> memory pressure since it's a network driver.
> 
> Since there are fixed number of work items, explicit concurrency
> limit is unnecessary here.
> 
> Signed-off-by: Bhaktipriya Shridhar 
> ---
>  drivers/net/usb/pegasus.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9bbe0161..1434e5d 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -1129,7 +1129,8 @@ static int pegasus_probe(struct usb_interface *intf,
>   return -ENODEV;
> 
>   if (pegasus_count == 0) {
> - pegasus_workqueue = create_singlethread_workqueue("pegasus");
> + pegasus_workqueue = alloc_workqueue("pegasus", WQ_MEM_RECLAIM,
> + 0);
>   if (!pegasus_workqueue)
>   return -ENOMEM;
>   }
> --
> 2.1.4

Nope, there is no need for singlethread-ness here.  As long as the flag you 
used 
is doing the right thing i am OK with the patch.


Petko

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 14:40:13, David Laight wrote:
> From: Linuxppc-dev 
> [mailto:linuxppc-dev-bounces+david.laight=aculab@lists.ozlabs.org] On 
> Behalf Of
> > > > So given what you have above, you'd use something like:
> > > >
> > > > struct ima_kexec_hdr {
> > > > u16 version;
> > > > u16 _reserved0;
> > > > u32 _reserved1;
> > > > u64 buffer_size;
> > > > u64 count;
> > > > };
> > > >
> > > > cheers
> > >
> > > Thanks, I'll make this change.
> > 
> > I would suggest:
> > 
> > struct ima_kexec_hdr {
> > u64 buffer_size;
> > u64 count;
> > u16 version;
> > };
> > 
> > and let the compiler add the proper padding, depending on the architecture. 
> >  On
> > 32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) 
> > while
> > retaining the same functionality.
> 
> .
> 
> That doesn't work for 32bit applications on 64bit hosts.

Which part won't work?

> The extra bytes will make 0 difference to the allocation cost and lots to the 
> processing.

An example?


Petko

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 14:40:13, David Laight wrote:
> From: Linuxppc-dev 
> [mailto:linuxppc-dev-bounces+david.laight=aculab@lists.ozlabs.org] On 
> Behalf Of
> > > > So given what you have above, you'd use something like:
> > > >
> > > > struct ima_kexec_hdr {
> > > > u16 version;
> > > > u16 _reserved0;
> > > > u32 _reserved1;
> > > > u64 buffer_size;
> > > > u64 count;
> > > > };
> > > >
> > > > cheers
> > >
> > > Thanks, I'll make this change.
> > 
> > I would suggest:
> > 
> > struct ima_kexec_hdr {
> > u64 buffer_size;
> > u64 count;
> > u16 version;
> > };
> > 
> > and let the compiler add the proper padding, depending on the architecture. 
> >  On
> > 32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) 
> > while
> > retaining the same functionality.
> 
> .
> 
> That doesn't work for 32bit applications on 64bit hosts.

Which part won't work?

> The extra bytes will make 0 difference to the allocation cost and lots to the 
> processing.

An example?


Petko

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 08:54:36, Mimi Zohar wrote:
> On Wed, 2016-08-10 at 19:52 +1000, Michael Ellerman wrote:
> > Thiago Jung Bauermann  writes:
> > 
> > > Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
> > >> Thiago Jung Bauermann  writes:
> > >> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > >> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > >> >> > Mimi Zohar  writes: 
> > >> >> > > +/* Some details preceding the binary serialized measurement list
> > >> >> > > */
> > >> >> > > +struct ima_kexec_hdr {
> > >> >> > > + unsigned short version;
> > >> >> > > + unsigned long buffer_size;
> > >> >> > > + unsigned long count;
> > >> >> > > +} __packed;
> > >> >> > > +
> > >> >> > 
> > >> >> > Am I understanding it correctly that this structure is passed 
> > >> >> > between
> > >> >> > kernels?
> > >> >> 
> > >> >> Yes, the header prefixes the measurement list, which is being passed 
> > >> >> on
> > >> >> the same computer to the next kernel.  Could the architecture (eg.
> > >> >> LE/BE) change between soft re-boots?
> > >> > 
> > >> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
> > >> > Whether we want to support that or not is another question...
> > >> 
> > >> Yes you must support that. BE -> LE and vice versa.
> > >
> > > I didn't test BE - LE yet, but will do.
> > 
> > Thanks.
> 
> Ok. There have been requests for making the binary_runtime_measurements
> architecture independent.  As this was not a network facing interface,
> we left it in native format.  With the kernel now consuming this data,
> it makes sense for the binary_runtime_measurements to be in an
> architecture independent format.
> 
> Unfortunately, as the /ima/binary_runtime_measurements is
> not prefixed with any metadata, this change would need to be Kconfig
> based, but kexec would always use the architecture independent format.
> 
> > >> You should also consider the possibility that the next kernel is not
> > >> Linux.
> 
> Oh!
> 
> > > If the next kernel is an ELF binary and it supports the kexec "calling 
> > > convention", it should work too. What could possibly go wrong? I can try 
> > > FreeBSD (I suppose it's an ELF kernel) and see what happens.
> > 
> > At least for old style kexec (not sys_kexec_load()) I don't think it
> > even needs to be an ELF binary.
> > 
> > I think there are folks working on FreeBSD (or $?BSD), so I think the
> > basic kexec part works.
> > 
> > There's nothing (yet) that wants to use this measurement list obviously,
> > but it should be designed such that it could be used by an unknown
> > future kernel that knows the ABI.
> > 
> > So given what you have above, you'd use something like:
> > 
> > struct ima_kexec_hdr {
> > u16 version;
> > u16 _reserved0;
> > u32 _reserved1;
> > u64 buffer_size;
> > u64 count;
> > };
> > 
> > cheers
> 
> Thanks, I'll make this change.

I would suggest:

struct ima_kexec_hdr {
u64 buffer_size;
u64 count;
u16 version;
};

and let the compiler add the proper padding, depending on the architecture.  On 
32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) while 
retaining the same functionality.


cheers,
Petko

Re: [Linux-ima-devel] [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-10 08:54:36, Mimi Zohar wrote:
> On Wed, 2016-08-10 at 19:52 +1000, Michael Ellerman wrote:
> > Thiago Jung Bauermann  writes:
> > 
> > > Am Mittwoch, 10 August 2016, 13:41:08 schrieb Michael Ellerman:
> > >> Thiago Jung Bauermann  writes:
> > >> > Am Dienstag, 09 August 2016, 09:01:13 schrieb Mimi Zohar:
> > >> >> On Tue, 2016-08-09 at 20:59 +1000, Michael Ellerman wrote:
> > >> >> > Mimi Zohar  writes: 
> > >> >> > > +/* Some details preceding the binary serialized measurement list
> > >> >> > > */
> > >> >> > > +struct ima_kexec_hdr {
> > >> >> > > + unsigned short version;
> > >> >> > > + unsigned long buffer_size;
> > >> >> > > + unsigned long count;
> > >> >> > > +} __packed;
> > >> >> > > +
> > >> >> > 
> > >> >> > Am I understanding it correctly that this structure is passed 
> > >> >> > between
> > >> >> > kernels?
> > >> >> 
> > >> >> Yes, the header prefixes the measurement list, which is being passed 
> > >> >> on
> > >> >> the same computer to the next kernel.  Could the architecture (eg.
> > >> >> LE/BE) change between soft re-boots?
> > >> > 
> > >> > Yes. I am able to boot a BE kernel from an LE kernel with my patches.
> > >> > Whether we want to support that or not is another question...
> > >> 
> > >> Yes you must support that. BE -> LE and vice versa.
> > >
> > > I didn't test BE - LE yet, but will do.
> > 
> > Thanks.
> 
> Ok. There have been requests for making the binary_runtime_measurements
> architecture independent.  As this was not a network facing interface,
> we left it in native format.  With the kernel now consuming this data,
> it makes sense for the binary_runtime_measurements to be in an
> architecture independent format.
> 
> Unfortunately, as the /ima/binary_runtime_measurements is
> not prefixed with any metadata, this change would need to be Kconfig
> based, but kexec would always use the architecture independent format.
> 
> > >> You should also consider the possibility that the next kernel is not
> > >> Linux.
> 
> Oh!
> 
> > > If the next kernel is an ELF binary and it supports the kexec "calling 
> > > convention", it should work too. What could possibly go wrong? I can try 
> > > FreeBSD (I suppose it's an ELF kernel) and see what happens.
> > 
> > At least for old style kexec (not sys_kexec_load()) I don't think it
> > even needs to be an ELF binary.
> > 
> > I think there are folks working on FreeBSD (or $?BSD), so I think the
> > basic kexec part works.
> > 
> > There's nothing (yet) that wants to use this measurement list obviously,
> > but it should be designed such that it could be used by an unknown
> > future kernel that knows the ABI.
> > 
> > So given what you have above, you'd use something like:
> > 
> > struct ima_kexec_hdr {
> > u16 version;
> > u16 _reserved0;
> > u32 _reserved1;
> > u64 buffer_size;
> > u64 count;
> > };
> > 
> > cheers
> 
> Thanks, I'll make this change.

I would suggest:

struct ima_kexec_hdr {
u64 buffer_size;
u64 count;
u16 version;
};

and let the compiler add the proper padding, depending on the architecture.  On 
32bit machine we'll have 4 bytes smaller allocations (compared to 64bit) while 
retaining the same functionality.


cheers,
Petko

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-05 09:34:38, Mimi Zohar wrote:
> Hi Petko,
> 
> Thank you for review!
> 
> On Fri, 2016-08-05 at 11:44 +0300, Petko Manolov wrote:
> > On 16-08-04 08:24:29, Mimi Zohar wrote:
> > > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > > of the running kernel must be saved and restored on boot.  This patch
> > > restores the measurement list.
> > > 
> > > Changelog:
> > > - call ima_load_kexec_buffer() (Thiago)
> > > 
> > > Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> > > ---
> > >  security/integrity/ima/Makefile   |   1 +
> > >  security/integrity/ima/ima.h  |  10 ++
> > >  security/integrity/ima/ima_init.c |   2 +
> > >  security/integrity/ima/ima_kexec.c|  55 +++
> > >  security/integrity/ima/ima_queue.c|  10 ++
> > >  security/integrity/ima/ima_template.c | 171 
> > > ++
> > >  6 files changed, 249 insertions(+)
> > >  create mode 100644 security/integrity/ima/ima_kexec.c
> > > 
> > > diff --git a/security/integrity/ima/Makefile 
> > > b/security/integrity/ima/Makefile
> > > index c34599f..c0ce7b1 100644
> > > --- a/security/integrity/ima/Makefile
> > > +++ b/security/integrity/ima/Makefile
> > > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> > >  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o 
> > > ima_api.o \
> > >ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
> > >  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > > +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
> > >  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index b5728da..84e8d36 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > >  };
> > >  extern struct list_head ima_measurements;/* list of all 
> > > measurements */
> > >  
> > > +/* Some details preceding the binary serialized measurement list */
> > > +struct ima_kexec_hdr {
> > > + unsigned short version;
> > > + unsigned long buffer_size;
> > > + unsigned long count;
> > > +} __packed;
> > 
> > Unless there is no real need for this structure to be packed i suggest 
> > dropping the attribute.  When referenced through pointer 32bit ARM and MIPS 
> > (and likely all other 32bit RISC CPUs) use rather inefficient byte loads 
> > and 
> > stores.
> > 
> > Worse, if, for example, ->count is going to be read/written concurrently 
> > from multiple threads we get torn loads/stores thus losing atomicity of the 
> > access.
> 
> This header is used to prefix the serialized binary measurement list with 
> some 
> meta-data about the measurement list being restored. Unfortunately 
> kexec_get_handover_buffer() returns the segment size, not the actual ima 
> measurement list buffer size.  The header info is set using memcpy() once in 
> ima_dump_measurement_list() and then the fields are used in 
> ima_restore_measurement_list() to verify the buffer.

As long as there is no concurrent reads/writes this should be OK.

> The binary runtime measurement list is packed, so the other two structures - 
> binary_hdr_v1 and binary_data_v1 - must be packed.  Does it make sense for 
> this header not to be packed as well?  Would copying the header fields to 
> local variables before being used solve your concern?

Copying to aligned variables would be necessary only if:

a) some sort of atomicity is needed, and/or
б) speed is of concern;

> Remember this code is used once on the kexec execute and again on reboot.

If we don't need a) _and_ b) then you don't need to bother.


Petko

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-05 09:34:38, Mimi Zohar wrote:
> Hi Petko,
> 
> Thank you for review!
> 
> On Fri, 2016-08-05 at 11:44 +0300, Petko Manolov wrote:
> > On 16-08-04 08:24:29, Mimi Zohar wrote:
> > > The TPM PCRs are only reset on a hard reboot.  In order to validate a
> > > TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> > > of the running kernel must be saved and restored on boot.  This patch
> > > restores the measurement list.
> > > 
> > > Changelog:
> > > - call ima_load_kexec_buffer() (Thiago)
> > > 
> > > Signed-off-by: Mimi Zohar 
> > > ---
> > >  security/integrity/ima/Makefile   |   1 +
> > >  security/integrity/ima/ima.h  |  10 ++
> > >  security/integrity/ima/ima_init.c |   2 +
> > >  security/integrity/ima/ima_kexec.c|  55 +++
> > >  security/integrity/ima/ima_queue.c|  10 ++
> > >  security/integrity/ima/ima_template.c | 171 
> > > ++
> > >  6 files changed, 249 insertions(+)
> > >  create mode 100644 security/integrity/ima/ima_kexec.c
> > > 
> > > diff --git a/security/integrity/ima/Makefile 
> > > b/security/integrity/ima/Makefile
> > > index c34599f..c0ce7b1 100644
> > > --- a/security/integrity/ima/Makefile
> > > +++ b/security/integrity/ima/Makefile
> > > @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
> > >  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o 
> > > ima_api.o \
> > >ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
> > >  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> > > +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
> > >  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > > index b5728da..84e8d36 100644
> > > --- a/security/integrity/ima/ima.h
> > > +++ b/security/integrity/ima/ima.h
> > > @@ -102,6 +102,13 @@ struct ima_queue_entry {
> > >  };
> > >  extern struct list_head ima_measurements;/* list of all 
> > > measurements */
> > >  
> > > +/* Some details preceding the binary serialized measurement list */
> > > +struct ima_kexec_hdr {
> > > + unsigned short version;
> > > + unsigned long buffer_size;
> > > + unsigned long count;
> > > +} __packed;
> > 
> > Unless there is no real need for this structure to be packed i suggest 
> > dropping the attribute.  When referenced through pointer 32bit ARM and MIPS 
> > (and likely all other 32bit RISC CPUs) use rather inefficient byte loads 
> > and 
> > stores.
> > 
> > Worse, if, for example, ->count is going to be read/written concurrently 
> > from multiple threads we get torn loads/stores thus losing atomicity of the 
> > access.
> 
> This header is used to prefix the serialized binary measurement list with 
> some 
> meta-data about the measurement list being restored. Unfortunately 
> kexec_get_handover_buffer() returns the segment size, not the actual ima 
> measurement list buffer size.  The header info is set using memcpy() once in 
> ima_dump_measurement_list() and then the fields are used in 
> ima_restore_measurement_list() to verify the buffer.

As long as there is no concurrent reads/writes this should be OK.

> The binary runtime measurement list is packed, so the other two structures - 
> binary_hdr_v1 and binary_data_v1 - must be packed.  Does it make sense for 
> this header not to be packed as well?  Would copying the header fields to 
> local variables before being used solve your concern?

Copying to aligned variables would be necessary only if:

a) some sort of atomicity is needed, and/or
б) speed is of concern;

> Remember this code is used once on the kexec execute and again on reboot.

If we don't need a) _and_ b) then you don't need to bother.


Petko

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-04 08:24:29, Mimi Zohar wrote:
> The TPM PCRs are only reset on a hard reboot.  In order to validate a
> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> of the running kernel must be saved and restored on boot.  This patch
> restores the measurement list.
> 
> Changelog:
> - call ima_load_kexec_buffer() (Thiago)
> 
> Signed-off-by: Mimi Zohar 
> ---
>  security/integrity/ima/Makefile   |   1 +
>  security/integrity/ima/ima.h  |  10 ++
>  security/integrity/ima/ima_init.c |   2 +
>  security/integrity/ima/ima_kexec.c|  55 +++
>  security/integrity/ima/ima_queue.c|  10 ++
>  security/integrity/ima/ima_template.c | 171 
> ++
>  6 files changed, 249 insertions(+)
>  create mode 100644 security/integrity/ima/ima_kexec.c
> 
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index c34599f..c0ce7b1 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
>ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
>  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
>  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b5728da..84e8d36 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -102,6 +102,13 @@ struct ima_queue_entry {
>  };
>  extern struct list_head ima_measurements;/* list of all measurements */
>  
> +/* Some details preceding the binary serialized measurement list */
> +struct ima_kexec_hdr {
> + unsigned short version;
> + unsigned long buffer_size;
> + unsigned long count;
> +} __packed;

Unless there is no real need for this structure to be packed i suggest dropping 
the attribute.  When referenced through pointer 32bit ARM and MIPS (and likely 
all other 32bit RISC CPUs) use rather inefficient byte loads and stores.

Worse, if, for example, ->count is going to be read/written concurrently from 
multiple threads we get torn loads/stores thus losing atomicity of the access.


Petko

Re: [PATCH 1/7] ima: on soft reboot, restore the measurement list

[Petko Manolov]

On 16-08-04 08:24:29, Mimi Zohar wrote:
> The TPM PCRs are only reset on a hard reboot.  In order to validate a
> TPM's quote after a soft reboot (eg. kexec -e), the IMA measurement list
> of the running kernel must be saved and restored on boot.  This patch
> restores the measurement list.
> 
> Changelog:
> - call ima_load_kexec_buffer() (Thiago)
> 
> Signed-off-by: Mimi Zohar 
> ---
>  security/integrity/ima/Makefile   |   1 +
>  security/integrity/ima/ima.h  |  10 ++
>  security/integrity/ima/ima_init.c |   2 +
>  security/integrity/ima/ima_kexec.c|  55 +++
>  security/integrity/ima/ima_queue.c|  10 ++
>  security/integrity/ima/ima_template.c | 171 
> ++
>  6 files changed, 249 insertions(+)
>  create mode 100644 security/integrity/ima/ima_kexec.c
> 
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index c34599f..c0ce7b1 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_IMA) += ima.o
>  ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
>ima_policy.o ima_template.o ima_template_lib.o ima_buffer.o
>  ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> +ima-$(CONFIG_KEXEC_FILE) += ima_kexec.o
>  obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b5728da..84e8d36 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -102,6 +102,13 @@ struct ima_queue_entry {
>  };
>  extern struct list_head ima_measurements;/* list of all measurements */
>  
> +/* Some details preceding the binary serialized measurement list */
> +struct ima_kexec_hdr {
> + unsigned short version;
> + unsigned long buffer_size;
> + unsigned long count;
> +} __packed;

Unless there is no real need for this structure to be packed i suggest dropping 
the attribute.  When referenced through pointer 32bit ARM and MIPS (and likely 
all other 32bit RISC CPUs) use rather inefficient byte loads and stores.

Worse, if, for example, ->count is going to be read/written concurrently from 
multiple threads we get torn loads/stores thus losing atomicity of the access.


Petko

Re: [PATCH] net: pegasus: remove unused variables and labels

[Petko Manolov]

Guys, come on.  This code is not dead.  This code is executed every time an 
ethernet packet is received.  It takes care of various error statistics. More 
importantly, it sends the actual (reported by the adapter) packet length to the 
network layer along with the packet.

This patch removes skb_put() and netif_rx() calls and effectively kills the RX 
path.  Not to mention that the driver was not even compiled before sending the 
patch upstream.

The only sensible, although cosmetic, change would be to replace:

if (!count || count < 4)

with

if (count < 4)

even though GCC takes care and it optimizes away "!count" condition.

Please revert this patch before Linus pulls from the network tree.


Petko



On 16-05-20 10:22:30, Arnd Bergmann wrote:
> The latest dead-code removal was slightly incomplete and
> left a few things behind that we now get compiler warnings
> for:
> 
> drivers/net/usb/pegasus.c: In function 'read_bulk_callback':
> drivers/net/usb/pegasus.c:475:1: error: label 'goon' defined but not used 
> [-Werror=unused-label]
> drivers/net/usb/pegasus.c:446:8: error: unused variable 'pkt_len' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:445:6: error: unused variable 'buf' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:443:17: error: unused variable 'count' 
> [-Werror=unused-variable]
> 
> This removes them as well.
> 
> Signed-off-by: Arnd Bergmann 
> Fixes: e00be9e4d0ff ("net: pegasus: remove dead coding")
> ---
>  drivers/net/usb/pegasus.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 1903d2e2b62d..df6d90ab7ca7 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -440,10 +440,8 @@ static void read_bulk_callback(struct urb *urb)
>  {
>   pegasus_t *pegasus = urb->context;
>   struct net_device *net;
> - int rx_status, count = urb->actual_length;
> + int rx_status;
>   int status = urb->status;
> - u8 *buf = urb->transfer_buffer;
> - __u16 pkt_len;
>  
>   if (!pegasus)
>   return;
> @@ -472,7 +470,6 @@ static void read_bulk_callback(struct urb *urb)
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
>   }
>  
> -goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> pegasus->rx_skb->data, PEGASUS_MTU,
> -- 
> 2.7.0
> 
> 

Re: [PATCH] net: pegasus: remove unused variables and labels

[Petko Manolov]

Guys, come on.  This code is not dead.  This code is executed every time an 
ethernet packet is received.  It takes care of various error statistics. More 
importantly, it sends the actual (reported by the adapter) packet length to the 
network layer along with the packet.

This patch removes skb_put() and netif_rx() calls and effectively kills the RX 
path.  Not to mention that the driver was not even compiled before sending the 
patch upstream.

The only sensible, although cosmetic, change would be to replace:

if (!count || count < 4)

with

if (count < 4)

even though GCC takes care and it optimizes away "!count" condition.

Please revert this patch before Linus pulls from the network tree.


Petko



On 16-05-20 10:22:30, Arnd Bergmann wrote:
> The latest dead-code removal was slightly incomplete and
> left a few things behind that we now get compiler warnings
> for:
> 
> drivers/net/usb/pegasus.c: In function 'read_bulk_callback':
> drivers/net/usb/pegasus.c:475:1: error: label 'goon' defined but not used 
> [-Werror=unused-label]
> drivers/net/usb/pegasus.c:446:8: error: unused variable 'pkt_len' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:445:6: error: unused variable 'buf' 
> [-Werror=unused-variable]
> drivers/net/usb/pegasus.c:443:17: error: unused variable 'count' 
> [-Werror=unused-variable]
> 
> This removes them as well.
> 
> Signed-off-by: Arnd Bergmann 
> Fixes: e00be9e4d0ff ("net: pegasus: remove dead coding")
> ---
>  drivers/net/usb/pegasus.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 1903d2e2b62d..df6d90ab7ca7 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -440,10 +440,8 @@ static void read_bulk_callback(struct urb *urb)
>  {
>   pegasus_t *pegasus = urb->context;
>   struct net_device *net;
> - int rx_status, count = urb->actual_length;
> + int rx_status;
>   int status = urb->status;
> - u8 *buf = urb->transfer_buffer;
> - __u16 pkt_len;
>  
>   if (!pegasus)
>   return;
> @@ -472,7 +470,6 @@ static void read_bulk_callback(struct urb *urb)
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
>   }
>  
> -goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> pegasus->rx_skb->data, PEGASUS_MTU,
> -- 
> 2.7.0
> 
> 

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-19 11:35:42, David Miller wrote:
> From: Heinrich Schuchardt 
> Date: Wed, 18 May 2016 02:13:30 +0200
> 
> > (!count || count < 4) is always true.
> > So let's remove the coding which is dead at least since 2005.
> > 
> > Signed-off-by: Heinrich Schuchardt 
> 
> Applied.

David, the patch you applied is broken.  It seems that you didn't follow the 
discussion from the past couple of days.  Please revert it.


Petko

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-19 11:35:42, David Miller wrote:
> From: Heinrich Schuchardt 
> Date: Wed, 18 May 2016 02:13:30 +0200
> 
> > (!count || count < 4) is always true.
> > So let's remove the coding which is dead at least since 2005.
> > 
> > Signed-off-by: Heinrich Schuchardt 
> 
> Applied.

David, the patch you applied is broken.  It seems that you didn't follow the 
discussion from the past couple of days.  Please revert it.


Petko

Re: [PATCH 1/1] net: pegasus: simplify logical constraint

[Petko Manolov]

On 16-05-18 20:40:51, Heinrich Schuchardt wrote:
> If !count is true, count < 4 is also true.

Yep, you're right.  However, gcc optimizes away the first condition.  What you 
really got me to think about is whether 4 is the right number.  I guess i shall 
refer to the HW documentation.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..9bbe0161 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -473,7 +473,7 @@ static void read_bulk_callback(struct urb *urb)
>   goto goon;
>   }
>  
> - if (!count || count < 4)
> + if (count < 4)
>   goto goon;
>  
>   rx_status = buf[count - 2];
> -- 
> 2.1.4
> 
> 

Re: [PATCH 1/1] net: pegasus: simplify logical constraint

[Petko Manolov]

On 16-05-18 20:40:51, Heinrich Schuchardt wrote:
> If !count is true, count < 4 is also true.

Yep, you're right.  However, gcc optimizes away the first condition.  What you 
really got me to think about is whether 4 is the right number.  I guess i shall 
refer to the HW documentation.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..9bbe0161 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -473,7 +473,7 @@ static void read_bulk_callback(struct urb *urb)
>   goto goon;
>   }
>  
> - if (!count || count < 4)
> + if (count < 4)
>   goto goon;
>  
>   rx_status = buf[count - 2];
> -- 
> 2.1.4
> 
> 

Re: [1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 09:15:40, Oliver Neukum wrote:
> On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> > On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > > (!count || count < 4) is always true.
> > 
> > Even if count >= 4 ?
> 
> The check for !count is redundant, though. Gcc, however,
> will surely simplify the expression.

Yep, gcc-6 generates this code:

...
cmp $0x3,%edx
jle b9 
...

Which does not invalidate your statement that "!count" is redundant. :)


Petko

Re: [1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 09:15:40, Oliver Neukum wrote:
> On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> > On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > > (!count || count < 4) is always true.
> > 
> > Even if count >= 4 ?
> 
> The check for !count is redundant, though. Gcc, however,
> will surely simplify the expression.

Yep, gcc-6 generates this code:

...
cmp $0x3,%edx
jle b9 
...

Which does not invalidate your statement that "!count" is redundant. :)


Petko

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 02:13:30, Heinrich Schuchardt wrote:
> (!count || count < 4) is always true.
> So let's remove the coding which is dead at least since 2005.

You may want to reconsider the above statement.  Just assume that 'count' is 
typically between 56 and 1514 bytes.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 53 
> ---
>  1 file changed, 53 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..1903d2e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
>   return;
>   default:
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
> - goto goon;
>   }
>  
> - if (!count || count < 4)
> - goto goon;
> -
> - rx_status = buf[count - 2];
> - if (rx_status & 0x1e) {
> - netif_dbg(pegasus, rx_err, net,
> -   "RX packet error %x\n", rx_status);
> - pegasus->stats.rx_errors++;
> - if (rx_status & 0x06)   /* long or runt */
> - pegasus->stats.rx_length_errors++;
> - if (rx_status & 0x08)
> - pegasus->stats.rx_crc_errors++;
> - if (rx_status & 0x10)   /* extra bits   */
> - pegasus->stats.rx_frame_errors++;
> - goto goon;
> - }
> - if (pegasus->chip == 0x8513) {
> - pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
> - pkt_len &= 0x0fff;
> - pegasus->rx_skb->data += 2;
> - } else {
> - pkt_len = buf[count - 3] << 8;
> - pkt_len += buf[count - 4];
> - pkt_len &= 0xfff;
> - pkt_len -= 4;
> - }
> -
> - /*
> -  * If the packet is unreasonably long, quietly drop it rather than
> -  * kernel panicing by calling skb_put.
> -  */
> - if (pkt_len > PEGASUS_MTU)
> - goto goon;
> -
> - /*
> -  * at this point we are sure pegasus->rx_skb != NULL
> -  * so we go ahead and pass up the packet.
> -  */
> - skb_put(pegasus->rx_skb, pkt_len);
> - pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
> - netif_rx(pegasus->rx_skb);
> - pegasus->stats.rx_packets++;
> - pegasus->stats.rx_bytes += pkt_len;
> -
> - if (pegasus->flags & PEGASUS_UNPLUG)
> - return;
> -
> - pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
> -   GFP_ATOMIC);
> -
> - if (pegasus->rx_skb == NULL)
> - goto tl_sched;
>  goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> -- 
> 2.1.4
> 
> 

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 02:13:30, Heinrich Schuchardt wrote:
> (!count || count < 4) is always true.
> So let's remove the coding which is dead at least since 2005.

You may want to reconsider the above statement.  Just assume that 'count' is 
typically between 56 and 1514 bytes.


Petko


> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/net/usb/pegasus.c | 53 
> ---
>  1 file changed, 53 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..1903d2e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
>   return;
>   default:
>   netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
> - goto goon;
>   }
>  
> - if (!count || count < 4)
> - goto goon;
> -
> - rx_status = buf[count - 2];
> - if (rx_status & 0x1e) {
> - netif_dbg(pegasus, rx_err, net,
> -   "RX packet error %x\n", rx_status);
> - pegasus->stats.rx_errors++;
> - if (rx_status & 0x06)   /* long or runt */
> - pegasus->stats.rx_length_errors++;
> - if (rx_status & 0x08)
> - pegasus->stats.rx_crc_errors++;
> - if (rx_status & 0x10)   /* extra bits   */
> - pegasus->stats.rx_frame_errors++;
> - goto goon;
> - }
> - if (pegasus->chip == 0x8513) {
> - pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
> - pkt_len &= 0x0fff;
> - pegasus->rx_skb->data += 2;
> - } else {
> - pkt_len = buf[count - 3] << 8;
> - pkt_len += buf[count - 4];
> - pkt_len &= 0xfff;
> - pkt_len -= 4;
> - }
> -
> - /*
> -  * If the packet is unreasonably long, quietly drop it rather than
> -  * kernel panicing by calling skb_put.
> -  */
> - if (pkt_len > PEGASUS_MTU)
> - goto goon;
> -
> - /*
> -  * at this point we are sure pegasus->rx_skb != NULL
> -  * so we go ahead and pass up the packet.
> -  */
> - skb_put(pegasus->rx_skb, pkt_len);
> - pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
> - netif_rx(pegasus->rx_skb);
> - pegasus->stats.rx_packets++;
> - pegasus->stats.rx_bytes += pkt_len;
> -
> - if (pegasus->flags & PEGASUS_UNPLUG)
> - return;
> -
> - pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
> -   GFP_ATOMIC);
> -
> - if (pegasus->rx_skb == NULL)
> - goto tl_sched;
>  goon:
>   usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
> usb_rcvbulkpipe(pegasus->usb, 1),
> -- 
> 2.1.4
> 
> 

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 16:07:00, David Howells wrote:
> Petko Manolov <pet...@mip-labs.com> wrote:
> 
> > > How about I change it to a choice-type item, with the following options:
> > > 
> > >  (1) No addition.
> > > 
> > >  (2) Addition restricted by built-in keyring.
> > > 
> > >  (3) Addition restricted by secondary keyring + built-in keyring.
> > > 
> > > where the second and third options then depend on the appropriate 
> > > keyrings 
> > > being enabled.
> > 
> > I would suggest leaving (1) and (3).  Since secondary keyring only accepts
> > keys signed by certificate in the system keyring I think (2) is redundant.
> > It adds extra complexity (Kconfig is vague enough already) while it doesn't
> > increase the overall security by much.
> 
> If I remove option (2), that would mean that if you want to allow keys to be 
> added to .ima if they're signed by the built-in keyring, then you also allow 
> keys to be added to .ima if they're signed by the secondary keyring if 
> enabled.

Exactly.  The primary difference between the built-in and secondary keyring is 
that the latter is R/W.  Chances are the user want either no addition or need 
dynamic key add/remove.

I don't have strong opinions against (2).  This is more of a discussion whether 
we should sacrifice in favor of simplicity or flexibility.

> Remember - these keyrings aren't necessarily restricted to IMA.

I am well aware of that.  At some point (perhaps not now) i'd like to discuss 
allowing kernel module loading based on keys in the secondary keyring.  It is a 
niche feature for those machines that have uptime measured in years.  I 
certainly don't expect it to be something the regular desktop or embedded users 
need.

Another issue that we left unresolved is the system-wide blacklist keyring.  It 
is at the same hierarchy level as the secondary keyring and serves a similar 
purpose although in opposite direction.


Petko

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 16:07:00, David Howells wrote:
> Petko Manolov  wrote:
> 
> > > How about I change it to a choice-type item, with the following options:
> > > 
> > >  (1) No addition.
> > > 
> > >  (2) Addition restricted by built-in keyring.
> > > 
> > >  (3) Addition restricted by secondary keyring + built-in keyring.
> > > 
> > > where the second and third options then depend on the appropriate 
> > > keyrings 
> > > being enabled.
> > 
> > I would suggest leaving (1) and (3).  Since secondary keyring only accepts
> > keys signed by certificate in the system keyring I think (2) is redundant.
> > It adds extra complexity (Kconfig is vague enough already) while it doesn't
> > increase the overall security by much.
> 
> If I remove option (2), that would mean that if you want to allow keys to be 
> added to .ima if they're signed by the built-in keyring, then you also allow 
> keys to be added to .ima if they're signed by the secondary keyring if 
> enabled.

Exactly.  The primary difference between the built-in and secondary keyring is 
that the latter is R/W.  Chances are the user want either no addition or need 
dynamic key add/remove.

I don't have strong opinions against (2).  This is more of a discussion whether 
we should sacrifice in favor of simplicity or flexibility.

> Remember - these keyrings aren't necessarily restricted to IMA.

I am well aware of that.  At some point (perhaps not now) i'd like to discuss 
allowing kernel module loading based on keys in the secondary keyring.  It is a 
niche feature for those machines that have uptime measured in years.  I 
certainly don't expect it to be something the regular desktop or embedded users 
need.

Another issue that we left unresolved is the system-wide blacklist keyring.  It 
is at the same hierarchy level as the secondary keyring and serves a similar 
purpose although in opposite direction.


Petko

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 14:44:24, David Howells wrote:
> Petko Manolov <pet...@mip-labs.com> wrote:
> 
> > I would suggest leaving (1) and (3).
> 
> Do you mean dropping option (2) and leaving (1) and (3)?  Or do you mean 
> dropping options (1) and (3)?

Dropping option (2) and leaving (1) and (3).

(2) is subset of (3) and IMHO adds unnecessary complexity.


Petko

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 14:44:24, David Howells wrote:
> Petko Manolov  wrote:
> 
> > I would suggest leaving (1) and (3).
> 
> Do you mean dropping option (2) and leaving (1) and (3)?  Or do you mean 
> dropping options (1) and (3)?

Dropping option (2) and leaving (1) and (3).

(2) is subset of (3) and IMHO adds unnecessary complexity.


Petko

Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]

[Petko Manolov]

On 16-03-08 13:13:59, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > but we're left with a lot of references to "system_trusted" (eg.
> > restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING
> 
> How about I pluralise it to SYSTEM_TRUSTED_KEYRINGS?  The fact that one is
> called builtin and the other secondary doesn't detract from the fact that
> they're both system-wide rings of trusted keys.
> 
> Or would you prefer .system_trusted_keys and .secondary_trusted_keys?  Even
> though the second is also a "system" trusted keyring.

Ah, naming things...  This is true science... :-)


cheers,
Petko

Re: [RFC PATCH 11/12] certs: Add a secondary system keyring that can be added to dynamically [ver #2]

[Petko Manolov]

On 16-03-08 13:13:59, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > but we're left with a lot of references to "system_trusted" (eg.
> > restrict_link_to_system_trusted, depends on SYSTEM_TRUSTED_KEYRING
> 
> How about I pluralise it to SYSTEM_TRUSTED_KEYRINGS?  The fact that one is
> called builtin and the other secondary doesn't detract from the fact that
> they're both system-wide rings of trusted keys.
> 
> Or would you prefer .system_trusted_keys and .secondary_trusted_keys?  Even
> though the second is also a "system" trusted keyring.

Ah, naming things...  This is true science... :-)


cheers,
Petko

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 13:08:36, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > Only certificates signed by a key on the system keyring were added to
> > the IMA keyring, unless IMA_MOK_KEYRING was configured.  Then, the
> > certificate could be signed by a either a key on the system or ima_mok
> > keyrings.  To replicate this behavior, the default behavior should be to
> > only permit certificates signed by a key on the builtin keyring, unless
> > this new Kconfig is enabled.  Only then, permit certificates signed by a
> > key on either the builtin or secondary keyrings to be added to the IMA
> > keyring.
> 
> How about I change it to a choice-type item, with the following options:
> 
>  (1) No addition.
> 
>  (2) Addition restricted by built-in keyring.
> 
>  (3) Addition restricted by secondary keyring + built-in keyring.
> 
> where the second and third options then depend on the appropriate keyrings 
> being enabled.

I would suggest leaving (1) and (3).  Since secondary keyring only accepts keys 
signed by certificate in the system keyring I think (2) is redundant.  It adds 
extra complexity (Kconfig is vague enough already) while it doesn't increase 
the 
overall security by much.


cheers,
Petko

Re: [RFC PATCH 12/12] IMA: Use the the system trusted keyrings instead of .ima_mok [ver #2]

[Petko Manolov]

On 16-03-08 13:08:36, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > Only certificates signed by a key on the system keyring were added to
> > the IMA keyring, unless IMA_MOK_KEYRING was configured.  Then, the
> > certificate could be signed by a either a key on the system or ima_mok
> > keyrings.  To replicate this behavior, the default behavior should be to
> > only permit certificates signed by a key on the builtin keyring, unless
> > this new Kconfig is enabled.  Only then, permit certificates signed by a
> > key on either the builtin or secondary keyrings to be added to the IMA
> > keyring.
> 
> How about I change it to a choice-type item, with the following options:
> 
>  (1) No addition.
> 
>  (2) Addition restricted by built-in keyring.
> 
>  (3) Addition restricted by secondary keyring + built-in keyring.
> 
> where the second and third options then depend on the appropriate keyrings 
> being enabled.

I would suggest leaving (1) and (3).  Since secondary keyring only accepts keys 
signed by certificate in the system keyring I think (2) is redundant.  It adds 
extra complexity (Kconfig is vague enough already) while it doesn't increase 
the 
overall security by much.


cheers,
Petko

Re: [RFC PATCH 00/20] KEYS: Restrict additions to 'trusted' keyrings [ver #2]

[Petko Manolov]

I assume this is intended for 4.6 or later, correct?


Petko


On 16-01-19 11:30:26, David Howells wrote:
> 
> Here's a set of patches that changes how certificates/keys are determined
> to be trusted.  That's currently a two-step process:
> 
>  (1) Up until recently, when an X.509 certificate was parsed - no matter
>  the source - it was judged against the keys in .system_keyring,
>  assuming those keys to be trusted if they have KEY_FLAG_TRUSTED set
>  upon them.
> 
>  This has just been changed such that any key in the .ima_mok keyring
>  may also be used to judge the trustwortiness of a new certificate,
>  whether or not the .ima_mok keyring is meant to be consulted for
>  whatever process is being undertaken.
> 
>  If a certificate is determined to be trustworthy, KEY_FLAG_TRUSTED
>  will be set upon a key it is loaded into (if it is loaded into one),
>  no matter what the key is going to be loaded for.
> 
>  (2) If an X.509 certificate is loaded into a key, then that key - if
>  KEY_FLAG_TRUSTED gets set upon it - can be linked into any keyring
>  with KEY_FLAG_TRUSTED_ONLY set upon it.  This was meant to be the
>  system keyring only, but has been extended to various IMA keyrings.
> 
>  A user can at will link any key marked KEY_FLAG_TRUSTED into any
>  keyring marked KEY_FLAG_TRUSTED_ONLY if the relevant permissions masks
>  permit it.
> 
> These patches change that:
> 
>  (1) Trust becomes a matter of consulting the ring of trusted keys supplied
>  when the trust is evaluated only.
> 
>  (2) Asymmetric keys retain the source certificate signature information
>  for future evaluation rather than discarding it.
> 
>  (3) Every keyring can be supplied with its own manager function to
>  restrict what may be added to that keyring.  This is called whenever a
>  key is to be linked into the keyring to guard against a key being
>  created in one keyring and then linked across.
> 
>  This function is supplied with the keyring and the key type and
>  payload[*] of the key being linked in for use in its evaluation.  It
>  is permitted to use other data also, such as the contents of other
>  keyrings such as the system keyrings.
> 
>  [*] The type and payload are supplied instead of a key because as an
>optimisation this function may be called whilst creating a key and
>so may reject the proposed key between preparse and allocation.
> 
>  (4) A default manager function is provided that permits keys to be
>  restricted to only asymmetric keys that are vouched for by the
>  contents of the system keyring.
> 
>  (5) A key allocation flag, KEY_ALLOC_BYPASS_RESTRICTION, is made available
>  so that the kernel can initialise keyrings with keys that form the
>  root of the trust relationship.
> 
>  (6) KEY_FLAG_TRUSTED and KEY_FLAG_TRUSTED_ONLY are removed, along with
>  key_preparsed_payload::trusted.
> 
> This change also makes it possible for userspace to create a private set of
> trusted keys and then to seal it by setting a manager function where the
> private set is wholly independent of the kernel's trust relationships.
> 
> Further changes in the set involve extracting certain IMA special keyrings
> and making them generally global:
> 
>  (*) .system_keyring is renamed to .builtin_trusted_keys and remains read
>  only.  It carries only keys built in to the kernel.  It may be where
>  UEFI keys should be loaded - though that could better be the new
>  secondary keyring (see below).
> 
>  (*) An optional system blacklist keyring is created to replace the IMA
>  keyring.
> 
>  (*) A 'blacklist' key type is created that may contain a hex string in
>  its description (it carries no payload).  When an X.509
>  certificate is parsed, the system blacklist is searched for a
>  blacklist key that matches the TBS hash of the X.509 certificate
>  and if one is found, the certificate is considered blacklisted.
> 
>  (*) A list of blacklisted hashes can be added to the system blacklist
>  keyring at compile time.  In the future it should also be possible
>  to load this up from such as the UEFI blacklist.
> 
>  (*) Keys can be added to the blacklist keyring by root if the keys are
>signed by a key in the builtin system keyring.  These can then be
>searched for by asymmetric key ID.  This allows the functionality
>of the IMA blacklist keyring to be replicated.
> 
>It might be worth making an asymmetric key subtype that carries no
>data to be used here as the cryptographic material is then just
>dead weight since the IDs are what matter.
> 
>  (*) An optional secondary system keyring (called .secondary_trusted_keys)
>  is added to replace the IMA MOK keyring.
> 
>  (*) Keys can be added to the secondary keyring by root if the keys can
>

Re: [RFC PATCH 00/20] KEYS: Restrict additions to 'trusted' keyrings [ver #2]

[Petko Manolov]

I assume this is intended for 4.6 or later, correct?


Petko


On 16-01-19 11:30:26, David Howells wrote:
> 
> Here's a set of patches that changes how certificates/keys are determined
> to be trusted.  That's currently a two-step process:
> 
>  (1) Up until recently, when an X.509 certificate was parsed - no matter
>  the source - it was judged against the keys in .system_keyring,
>  assuming those keys to be trusted if they have KEY_FLAG_TRUSTED set
>  upon them.
> 
>  This has just been changed such that any key in the .ima_mok keyring
>  may also be used to judge the trustwortiness of a new certificate,
>  whether or not the .ima_mok keyring is meant to be consulted for
>  whatever process is being undertaken.
> 
>  If a certificate is determined to be trustworthy, KEY_FLAG_TRUSTED
>  will be set upon a key it is loaded into (if it is loaded into one),
>  no matter what the key is going to be loaded for.
> 
>  (2) If an X.509 certificate is loaded into a key, then that key - if
>  KEY_FLAG_TRUSTED gets set upon it - can be linked into any keyring
>  with KEY_FLAG_TRUSTED_ONLY set upon it.  This was meant to be the
>  system keyring only, but has been extended to various IMA keyrings.
> 
>  A user can at will link any key marked KEY_FLAG_TRUSTED into any
>  keyring marked KEY_FLAG_TRUSTED_ONLY if the relevant permissions masks
>  permit it.
> 
> These patches change that:
> 
>  (1) Trust becomes a matter of consulting the ring of trusted keys supplied
>  when the trust is evaluated only.
> 
>  (2) Asymmetric keys retain the source certificate signature information
>  for future evaluation rather than discarding it.
> 
>  (3) Every keyring can be supplied with its own manager function to
>  restrict what may be added to that keyring.  This is called whenever a
>  key is to be linked into the keyring to guard against a key being
>  created in one keyring and then linked across.
> 
>  This function is supplied with the keyring and the key type and
>  payload[*] of the key being linked in for use in its evaluation.  It
>  is permitted to use other data also, such as the contents of other
>  keyrings such as the system keyrings.
> 
>  [*] The type and payload are supplied instead of a key because as an
>optimisation this function may be called whilst creating a key and
>so may reject the proposed key between preparse and allocation.
> 
>  (4) A default manager function is provided that permits keys to be
>  restricted to only asymmetric keys that are vouched for by the
>  contents of the system keyring.
> 
>  (5) A key allocation flag, KEY_ALLOC_BYPASS_RESTRICTION, is made available
>  so that the kernel can initialise keyrings with keys that form the
>  root of the trust relationship.
> 
>  (6) KEY_FLAG_TRUSTED and KEY_FLAG_TRUSTED_ONLY are removed, along with
>  key_preparsed_payload::trusted.
> 
> This change also makes it possible for userspace to create a private set of
> trusted keys and then to seal it by setting a manager function where the
> private set is wholly independent of the kernel's trust relationships.
> 
> Further changes in the set involve extracting certain IMA special keyrings
> and making them generally global:
> 
>  (*) .system_keyring is renamed to .builtin_trusted_keys and remains read
>  only.  It carries only keys built in to the kernel.  It may be where
>  UEFI keys should be loaded - though that could better be the new
>  secondary keyring (see below).
> 
>  (*) An optional system blacklist keyring is created to replace the IMA
>  keyring.
> 
>  (*) A 'blacklist' key type is created that may contain a hex string in
>  its description (it carries no payload).  When an X.509
>  certificate is parsed, the system blacklist is searched for a
>  blacklist key that matches the TBS hash of the X.509 certificate
>  and if one is found, the certificate is considered blacklisted.
> 
>  (*) A list of blacklisted hashes can be added to the system blacklist
>  keyring at compile time.  In the future it should also be possible
>  to load this up from such as the UEFI blacklist.
> 
>  (*) Keys can be added to the blacklist keyring by root if the keys are
>signed by a key in the builtin system keyring.  These can then be
>searched for by asymmetric key ID.  This allows the functionality
>of the IMA blacklist keyring to be replicated.
> 
>It might be worth making an asymmetric key subtype that carries no
>data to be used here as the cryptographic material is then just
>dead weight since the IDs are what matter.
> 
>  (*) An optional secondary system keyring (called .secondary_trusted_keys)
>  is added to replace the IMA MOK keyring.
> 
>  (*) Keys can be added to the secondary keyring by root if the keys can
>

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Petko Manolov]

On 16-01-06 13:21:27, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > The x509_validate_trust() was originally added for IMA to ensure, on a 
> > secure boot system, a certificate chain of trust rooted in hardware. The 
> > IMA 
> > MOK keyring extends this certificate chain of trust to the running system.
> 
> The problem is that because 'trusted' is a boolean, a key in the IMA MOK 
> keyring will permit addition to the system keyring.

If this is true the i am clearly doing the wrong thing.  The CA hierarchy 
should 
run top-bottom, not the other way around.

IMA MOK was introduced mainly because .system keyring was static at the time.  
Assuming i have my root certificate in .system how can i add more keys to this 
keyring?  The new keys have been signed by my root CA?  Is this possible since 
your October patch-set or i've been missing something this whole time?


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Petko Manolov]

On 16-01-06 13:21:27, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > The x509_validate_trust() was originally added for IMA to ensure, on a 
> > secure boot system, a certificate chain of trust rooted in hardware. The 
> > IMA 
> > MOK keyring extends this certificate chain of trust to the running system.
> 
> The problem is that because 'trusted' is a boolean, a key in the IMA MOK 
> keyring will permit addition to the system keyring.

If this is true the i am clearly doing the wrong thing.  The CA hierarchy 
should 
run top-bottom, not the other way around.

IMA MOK was introduced mainly because .system keyring was static at the time.  
Assuming i have my root certificate in .system how can i add more keys to this 
keyring?  The new keys have been signed by my root CA?  Is this possible since 
your October patch-set or i've been missing something this whole time?


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Petko Manolov]

On 16-01-05 16:40:31, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > You're missing Petko's patch:
> > 41c89b6 IMA: create machine owner and blacklist keyrings
> 
> It should also be cc'd to the keyrings mailing list.

Right.

If i am not terribly mistaken there's no way to revoke a certificate that is in 
a CA hierarchy with the system keyring on top of it.  Certain scenarios require 
us to revoke them as it was presented at the last year's LSS.

If x509_key_preparse() is not the right place then where shall i place the 
check?


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH] X.509: Don't check the signature on apparently self-signed keys [ver #2]

[Petko Manolov]

On 16-01-05 16:40:31, David Howells wrote:
> Mimi Zohar  wrote:
> 
> > You're missing Petko's patch:
> > 41c89b6 IMA: create machine owner and blacklist keyrings
> 
> It should also be cc'd to the keyrings mailing list.

Right.

If i am not terribly mistaken there's no way to revoke a certificate that is in 
a CA hierarchy with the system keyring on top of it.  Certain scenarios require 
us to revoke them as it was presented at the last year's LSS.

If x509_key_preparse() is not the right place then where shall i place the 
check?


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 16-01-01 04:34:16, Al Viro wrote:
> On Thu, Dec 31, 2015 at 12:45:35PM +0200, Petko Manolov wrote:
> 
> > I introduced the write mutex when ima_write_policy() stopped being 
> > serialized by 
> > other means.  Come to think about it the semaphore could be taken right 
> > before 
> > copy_from_user() so it is my fault, not Stephen's.
> 
> s/before/after/, surely?

Right.  This is a quick patch which i hope solves most issues...


Petko


>From 6c9058009c59fda5b8e98a3fc09497ce3efdb3e9 Mon Sep 17 00:00:00 2001
From: Petko Manolov 
Date: Fri, 1 Jan 2016 19:10:43 +0200
Subject: [PATCH] ima_write_policy() optimizations;

There is no need to hold the write semaphore for so long.  We only need it
around ima_parse_add_rule();

The return path now takes into account failed kmalloc() call.

Signed-off-by: Petko Manolov 
---
 security/integrity/ima/ima_fs.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 3caed6d..d2c0d55 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -261,13 +261,7 @@ static const struct file_operations 
ima_ascii_measurements_ops = {
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
size_t datalen, loff_t *ppos)
 {
-   char *data = NULL;
ssize_t result;
-   int res;
-
-   res = mutex_lock_interruptible(_write_mutex);
-   if (res)
-   return res;
 
if (datalen >= PAGE_SIZE)
datalen = PAGE_SIZE - 1;
@@ -286,15 +280,21 @@ static ssize_t ima_write_policy(struct file *file, const 
char __user *buf,
 
result = -EFAULT;
if (copy_from_user(data, buf, datalen))
-   goto out;
+   goto out_free;
+
+   result = mutex_lock_interruptible(_write_mutex);
+   if (result)
+   goto out_free;
 
result = ima_parse_add_rule(data);
-out:
+
+   mutex_unlock(_write_mutex);
+
if (result < 0)
valid_policy = 0;
+out_free:
kfree(data);
-   mutex_unlock(_write_mutex);
-
+out:
return result;
 }
 
-- 
2.6.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 16-01-01 04:34:16, Al Viro wrote:
> On Thu, Dec 31, 2015 at 12:45:35PM +0200, Petko Manolov wrote:
> 
> > I introduced the write mutex when ima_write_policy() stopped being 
> > serialized by 
> > other means.  Come to think about it the semaphore could be taken right 
> > before 
> > copy_from_user() so it is my fault, not Stephen's.
> 
> s/before/after/, surely?

Right.  This is a quick patch which i hope solves most issues...


Petko


>From 6c9058009c59fda5b8e98a3fc09497ce3efdb3e9 Mon Sep 17 00:00:00 2001
From: Petko Manolov <pet...@mip-labs.com>
Date: Fri, 1 Jan 2016 19:10:43 +0200
Subject: [PATCH] ima_write_policy() optimizations;

There is no need to hold the write semaphore for so long.  We only need it
around ima_parse_add_rule();

The return path now takes into account failed kmalloc() call.

Signed-off-by: Petko Manolov <pet...@mip-labs.com>
---
 security/integrity/ima/ima_fs.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 3caed6d..d2c0d55 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -261,13 +261,7 @@ static const struct file_operations 
ima_ascii_measurements_ops = {
 static ssize_t ima_write_policy(struct file *file, const char __user *buf,
size_t datalen, loff_t *ppos)
 {
-   char *data = NULL;
ssize_t result;
-   int res;
-
-   res = mutex_lock_interruptible(_write_mutex);
-   if (res)
-   return res;
 
if (datalen >= PAGE_SIZE)
datalen = PAGE_SIZE - 1;
@@ -286,15 +280,21 @@ static ssize_t ima_write_policy(struct file *file, const 
char __user *buf,
 
result = -EFAULT;
if (copy_from_user(data, buf, datalen))
-   goto out;
+   goto out_free;
+
+   result = mutex_lock_interruptible(_write_mutex);
+   if (result)
+   goto out_free;
 
result = ima_parse_add_rule(data);
-out:
+
+   mutex_unlock(_write_mutex);
+
if (result < 0)
valid_policy = 0;
+out_free:
kfree(data);
-   mutex_unlock(_write_mutex);
-
+out:
return result;
 }
 
-- 
2.6.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 15-12-31 04:30:19, Al Viro wrote:
> On Thu, Dec 31, 2015 at 03:24:53PM +1100, Stephen Rothwell wrote:
> > Hi James,
> > 
> > Today's linux-next merge of the security tree got a conflict in:
> > 
> >   security/integrity/ima/ima_fs.c
> > 
> > between commit:
> > 
> >   3bc8f29b149e ("new helper: memdup_user_nul()")
> > 
> > from the vfs tree and commit:
> > 
> >   38d859f991f3 ("IMA: policy can now be updated multiple times")
> > 
> > from the security tree.
> > 
> > I fixed it up (hopefully, see below) and can carry the fix as necessary
> > (no action is required).
>  
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> >   
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> >   
> > /* No partial writes. */
> > +   result = -EINVAL;
> > if (*ppos != 0)
> > -   return -EINVAL;
> > +   goto out;
> >   
> >  -  result = -ENOMEM;
> >  -  data = kmalloc(datalen + 1, GFP_KERNEL);
> >  -  if (!data)
> >  -  goto out;
> >  -
> >  -  *(data + datalen) = '\0';
> >  -
> >  -  result = -EFAULT;
> >  -  if (copy_from_user(data, buf, datalen))
> >  +  data = memdup_user_nul(buf, datalen);
> > -   if (IS_ERR(data))
> > -   return PTR_ERR(data);
> > ++  if (IS_ERR(data)) {
> > ++  result = PTR_ERR(data);
> > +   goto out;
> > ++  }
> 
> Why do it in this order?  With or without opencoding memdup_user_nul(),
> what's the point of taking the mutex before copying the data from
> userland?  All it achieves is holding it longer, over the area that
> needs no exclusion whatsoever.

I introduced the write mutex when ima_write_policy() stopped being serialized 
by 
other means.  Come to think about it the semaphore could be taken right before 
copy_from_user() so it is my fault, not Stephen's.

The patch, however, leaves out a bug where free without allocation can occur.  
Look at *ppos evaluation.  Instead of "goto out" it should be "return 
-EINVAL;".  
This requires the mutex lock to be moved down, though.


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: linux-next: manual merge of the security tree with the vfs tree

[Petko Manolov]

On 15-12-31 04:30:19, Al Viro wrote:
> On Thu, Dec 31, 2015 at 03:24:53PM +1100, Stephen Rothwell wrote:
> > Hi James,
> > 
> > Today's linux-next merge of the security tree got a conflict in:
> > 
> >   security/integrity/ima/ima_fs.c
> > 
> > between commit:
> > 
> >   3bc8f29b149e ("new helper: memdup_user_nul()")
> > 
> > from the vfs tree and commit:
> > 
> >   38d859f991f3 ("IMA: policy can now be updated multiple times")
> > 
> > from the security tree.
> > 
> > I fixed it up (hopefully, see below) and can carry the fix as necessary
> > (no action is required).
>  
> > +   res = mutex_lock_interruptible(_write_mutex);
> > +   if (res)
> > +   return res;
> >   
> > if (datalen >= PAGE_SIZE)
> > datalen = PAGE_SIZE - 1;
> >   
> > /* No partial writes. */
> > +   result = -EINVAL;
> > if (*ppos != 0)
> > -   return -EINVAL;
> > +   goto out;
> >   
> >  -  result = -ENOMEM;
> >  -  data = kmalloc(datalen + 1, GFP_KERNEL);
> >  -  if (!data)
> >  -  goto out;
> >  -
> >  -  *(data + datalen) = '\0';
> >  -
> >  -  result = -EFAULT;
> >  -  if (copy_from_user(data, buf, datalen))
> >  +  data = memdup_user_nul(buf, datalen);
> > -   if (IS_ERR(data))
> > -   return PTR_ERR(data);
> > ++  if (IS_ERR(data)) {
> > ++  result = PTR_ERR(data);
> > +   goto out;
> > ++  }
> 
> Why do it in this order?  With or without opencoding memdup_user_nul(),
> what's the point of taking the mutex before copying the data from
> userland?  All it achieves is holding it longer, over the area that
> needs no exclusion whatsoever.

I introduced the write mutex when ima_write_policy() stopped being serialized 
by 
other means.  Come to think about it the semaphore could be taken right before 
copy_from_user() so it is my fault, not Stephen's.

The patch, however, leaves out a bug where free without allocation can occur.  
Look at *ppos evaluation.  Instead of "goto out" it should be "return 
-EINVAL;".  
This requires the mutex lock to be moved down, though.


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] IMA: policy can be updated zero times

[Petko Manolov]

On 15-12-22 16:50:01, Sasha Levin wrote:
> On 12/22/2015 04:40 PM, Petko Manolov wrote:
> >> Thanks, Sasha.  By the time ima_update_policy() is called
> >> >ima_release_policy() has already output the policy update status
> >> >message.  I guess an empty policy could be considered a valid policy.
> >> >Could you add a msg indicating that the new policy was empty?
> > 
> > As far as I can say we can't get to ima_update_policy() with empty 
> > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in 
> > case 
> > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > too.
> 
> This is based on an actual crash rather than code analysis.

I was able to reproduce the crash with: echo "" > 
/sys/kernel/security/ima/policy

It turns out ima_parse_add_rule() returns 1, even though the string is empty 
This logic may be part of "empty policy is a valid policy" or something else.  
As it is more dangerous to change the behavior at this point i assume your 
patch 
is the right solution for the problem.

Acked-by: Petko Manolov 

Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
much work?


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] IMA: policy can be updated zero times

[Petko Manolov]

On 15-12-22 16:50:01, Sasha Levin wrote:
> On 12/22/2015 04:40 PM, Petko Manolov wrote:
> >> Thanks, Sasha.  By the time ima_update_policy() is called
> >> >ima_release_policy() has already output the policy update status
> >> >message.  I guess an empty policy could be considered a valid policy.
> >> >Could you add a msg indicating that the new policy was empty?
> > 
> > As far as I can say we can't get to ima_update_policy() with empty 
> > ima_temp_rules because ima_write_policy() will set valid_policy to 0 in 
> > case 
> > of an empty rule.  I'll double check it tomorrow, but please you do that 
> > too.
> 
> This is based on an actual crash rather than code analysis.

I was able to reproduce the crash with: echo "" > 
/sys/kernel/security/ima/policy

It turns out ima_parse_add_rule() returns 1, even though the string is empty 
This logic may be part of "empty policy is a valid policy" or something else.  
As it is more dangerous to change the behavior at this point i assume your 
patch 
is the right solution for the problem.

Acked-by: Petko Manolov <pet...@mip-labs.com>

Mimi, shall we change ima_parse_add_rule's behavior in the future or it's too 
much work?


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] IMA: policy can be updated zero times

[Petko Manolov]

On December 22, 2015 9:56:28 PM GMT+02:00, Mimi Zohar 
 wrote:
>On Tue, 2015-12-22 at 08:51 -0500, Sasha Levin wrote:
>> Commit "IMA: policy can now be updated multiple times" assumed that
>the
>> policy would be updated at least once.
>> 
>> If there are zero updates, the temporary list head object will get
>added
>> to the policy list, and later dereferenced as an IMA policy object,
>which
>> means that invalid memory will be accessed.
>> 
>> Signed-off-by: Sasha Levin 
>> ---
>>  security/integrity/ima/ima_policy.c |3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/security/integrity/ima/ima_policy.c
>b/security/integrity/ima/ima_policy.c
>> index ba5d2fc..9b958b8 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -431,6 +431,9 @@ void ima_update_policy(void)
>>  {
>>  struct list_head *first, *last, *policy;
>> 
>> +if (list_empty(_temp_rules))
>> +return;
>> +
>>  /* append current policy with the new rules */
>>  first = (_temp_rules)->next;
>>  last = (_temp_rules)->prev;
>
>Thanks, Sasha.  By the time ima_update_policy() is called
>ima_release_policy() has already output the policy update status
>message.  I guess an empty policy could be considered a valid policy.
>Could you add a msg indicating that the new policy was empty?


As far as I can say we can't get to ima_update_policy() with empty 
ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case of 
an empty rule.  I'll double check it tomorrow, but please you do that too.


cheers,
Petko




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] IMA: policy can be updated zero times

[Petko Manolov]

On December 22, 2015 9:56:28 PM GMT+02:00, Mimi Zohar 
 wrote:
>On Tue, 2015-12-22 at 08:51 -0500, Sasha Levin wrote:
>> Commit "IMA: policy can now be updated multiple times" assumed that
>the
>> policy would be updated at least once.
>> 
>> If there are zero updates, the temporary list head object will get
>added
>> to the policy list, and later dereferenced as an IMA policy object,
>which
>> means that invalid memory will be accessed.
>> 
>> Signed-off-by: Sasha Levin 
>> ---
>>  security/integrity/ima/ima_policy.c |3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/security/integrity/ima/ima_policy.c
>b/security/integrity/ima/ima_policy.c
>> index ba5d2fc..9b958b8 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -431,6 +431,9 @@ void ima_update_policy(void)
>>  {
>>  struct list_head *first, *last, *policy;
>> 
>> +if (list_empty(_temp_rules))
>> +return;
>> +
>>  /* append current policy with the new rules */
>>  first = (_temp_rules)->next;
>>  last = (_temp_rules)->prev;
>
>Thanks, Sasha.  By the time ima_update_policy() is called
>ima_release_policy() has already output the policy update status
>message.  I guess an empty policy could be considered a valid policy.
>Could you add a msg indicating that the new policy was empty?


As far as I can say we can't get to ima_update_policy() with empty 
ima_temp_rules because ima_write_policy() will set valid_policy to 0 in case of 
an empty rule.  I'll double check it tomorrow, but please you do that too.


cheers,
Petko




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring

[Petko Manolov]

On 15-10-23 14:43:53, Mimi Zohar wrote:
> On Fri, 2015-10-23 at 16:05 +0300, Petko Manolov wrote:
> > On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> 
> > > diff --git a/security/integrity/ima/Kconfig 
> > > b/security/integrity/ima/Kconfig
> > > index df30334..a292b88 100644
> > > --- a/security/integrity/ima/Kconfig
> > > +++ b/security/integrity/ima/Kconfig
> > > @@ -123,14 +123,17 @@ config IMA_APPRAISE
> > > If unsure, say N.
> > >  
> > >  config IMA_TRUSTED_KEYRING
> > > - bool "Require all keys on the .ima keyring be signed"
> > > + bool "Require all keys on the .ima keyring be signed (deprecated)"
> > >   depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> > >   depends on INTEGRITY_ASYMMETRIC_KEYS
> > > + select INTEGRITY_TRUSTED_KEYRING
> > >   default y
> > >   help
> > >  This option requires that all keys added to the .ima
> > >  keyring be signed by a key on the system trusted keyring.
> > >  
> > > +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> > > +
> > >  config IMA_LOAD_X509
> > >   bool "Load X509 certificate onto the '.ima' trusted keyring"
> > >   depends on IMA_TRUSTED_KEYRING
> > 
> > 
> > I guess we may as well remove this switch.  Otherwise somebody have to 
> > remember 
> > to post a patch that does so a few kernel releases after this one goes 
> > mainline.
> 
> There's no harm in leaving the "IMA_TRUSTED_KEYRING" Kconfig option for a 
> couple of releases (or perhaps until it is enabled in a long term release), 
> so 
> that the INTEGRITY_TRUSTED_KEYRING Kconfig option is enabled automatically.

I have no strong opinion either way.  Just saying.  Let it be for the moment.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring

[Petko Manolov]

On 15-10-23 14:43:53, Mimi Zohar wrote:
> On Fri, 2015-10-23 at 16:05 +0300, Petko Manolov wrote:
> > On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> 
> > > diff --git a/security/integrity/ima/Kconfig 
> > > b/security/integrity/ima/Kconfig
> > > index df30334..a292b88 100644
> > > --- a/security/integrity/ima/Kconfig
> > > +++ b/security/integrity/ima/Kconfig
> > > @@ -123,14 +123,17 @@ config IMA_APPRAISE
> > > If unsure, say N.
> > >  
> > >  config IMA_TRUSTED_KEYRING
> > > - bool "Require all keys on the .ima keyring be signed"
> > > + bool "Require all keys on the .ima keyring be signed (deprecated)"
> > >   depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
> > >   depends on INTEGRITY_ASYMMETRIC_KEYS
> > > + select INTEGRITY_TRUSTED_KEYRING
> > >   default y
> > >   help
> > >  This option requires that all keys added to the .ima
> > >  keyring be signed by a key on the system trusted keyring.
> > >  
> > > +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> > > +
> > >  config IMA_LOAD_X509
> > >   bool "Load X509 certificate onto the '.ima' trusted keyring"
> > >   depends on IMA_TRUSTED_KEYRING
> > 
> > 
> > I guess we may as well remove this switch.  Otherwise somebody have to 
> > remember 
> > to post a patch that does so a few kernel releases after this one goes 
> > mainline.
> 
> There's no harm in leaving the "IMA_TRUSTED_KEYRING" Kconfig option for a 
> couple of releases (or perhaps until it is enabled in a long term release), 
> so 
> that the INTEGRITY_TRUSTED_KEYRING Kconfig option is enabled automatically.

I have no strong opinion either way.  Just saying.  Let it be for the moment.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring

[Petko Manolov]

On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> Require all keys added to the EVM keyring be signed by an
> existing trusted key on the system trusted keyring.
> 
> This patch also switches IMA to use integrity_init_keyring().
> 
> Changes in v3:
> * Added 'init_keyring' config based variable to skip initializing
>   keyring instead of using  __integrity_init_keyring() wrapper.
> * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING
> 
> Changes in v2:
> * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common
>   CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option
> * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config
>   file compatibility. (Mimi Zohar)
> 
> Signed-off-by: Dmitry Kasatkin 
> ---
>  security/integrity/Kconfig| 11 +++
>  security/integrity/digsig.c   | 14 --
>  security/integrity/evm/evm_main.c |  8 +---
>  security/integrity/ima/Kconfig|  5 -
>  security/integrity/ima/ima.h  | 12 
>  security/integrity/ima/ima_init.c |  2 +-
>  security/integrity/integrity.h|  5 ++---
>  7 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 73c457b..21d7568 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS
> This option enables digital signature verification using
> asymmetric keys.
>  
> +config INTEGRITY_TRUSTED_KEYRING
> + bool "Require all keys on the integrity keyrings be signed"
> + depends on SYSTEM_TRUSTED_KEYRING
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + select KEYS_DEBUG_PROC_KEYS
> + default y
> + help
> +This option requires that all keys added to the .ima and
> +.evm keyrings be signed by a key on the system trusted
> +keyring.
> +
>  config INTEGRITY_AUDIT
>   bool "Enables integrity auditing support "
>   depends on AUDIT
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5be9ffb..8ef1511 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -24,15 +24,22 @@
>  static struct key *keyring[INTEGRITY_KEYRING_MAX];
>  
>  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING
>   "_evm",
> - "_module",
> -#ifndef CONFIG_IMA_TRUSTED_KEYRING
>   "_ima",
>  #else
> + ".evm",
>   ".ima",
>  #endif
> + "_module",
>  };
>  
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> +static bool init_keyring __initdata = true;
> +#else
> +static bool init_keyring __initdata;
> +#endif
> +
>  int integrity_digsig_verify(const unsigned int id, const char *sig, int 
> siglen,
>   const char *digest, int digestlen)
>  {
> @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id)
>   const struct cred *cred = current_cred();
>   int err = 0;
>  
> + if (!init_keyring)
> + return 0;
> +
>   keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
>   KGIDT_INIT(0), cred,
>   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> diff --git a/security/integrity/evm/evm_main.c 
> b/security/integrity/evm/evm_main.c
> index 1334e02..75b7e30 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,15 +478,17 @@ static int __init init_evm(void)
>  
>   evm_init_config();
>  
> + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
> + if (error)
> + return error;
> +
>   error = evm_init_secfs();
>   if (error < 0) {
>   pr_info("Error registering secfs\n");
> - goto err;
> + return error;
>   }
>  
>   return 0;
> -err:
> - return error;
>  }
>  
>  /*
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..a292b88 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -123,14 +123,17 @@ config IMA_APPRAISE
> If unsure, say N.
>  
>  config IMA_TRUSTED_KEYRING
> - bool "Require all keys on the .ima keyring be signed"
> + bool "Require all keys on the .ima keyring be signed (deprecated)"
>   depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
>   depends on INTEGRITY_ASYMMETRIC_KEYS
> + select INTEGRITY_TRUSTED_KEYRING
>   default y
>   help
>  This option requires that all keys added to the .ima
>  keyring be signed by a key on the system trusted keyring.
>  
> +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> +
>  config IMA_LOAD_X509
>   bool "Load X509 certificate onto the '.ima' trusted keyring"
>   depends on IMA_TRUSTED_KEYRING


I guess we may as well remove this switch.  Otherwise somebody have to remember 
to post a patch that does so a few kernel releases after this one goes 

Re: [PATCHv3 1/6] integrity: define '.evm' as a builtin 'trusted' keyring

[Petko Manolov]

On 15-10-22 21:49:25, Dmitry Kasatkin wrote:
> Require all keys added to the EVM keyring be signed by an
> existing trusted key on the system trusted keyring.
> 
> This patch also switches IMA to use integrity_init_keyring().
> 
> Changes in v3:
> * Added 'init_keyring' config based variable to skip initializing
>   keyring instead of using  __integrity_init_keyring() wrapper.
> * Added dependency back to CONFIG_IMA_TRUSTED_KEYRING
> 
> Changes in v2:
> * Replace CONFIG_EVM_TRUSTED_KEYRING with IMA and EVM common
>   CONFIG_INTEGRITY_TRUSTED_KEYRING configuration option
> * Deprecate CONFIG_IMA_TRUSTED_KEYRING but keep it for config
>   file compatibility. (Mimi Zohar)
> 
> Signed-off-by: Dmitry Kasatkin 
> ---
>  security/integrity/Kconfig| 11 +++
>  security/integrity/digsig.c   | 14 --
>  security/integrity/evm/evm_main.c |  8 +---
>  security/integrity/ima/Kconfig|  5 -
>  security/integrity/ima/ima.h  | 12 
>  security/integrity/ima/ima_init.c |  2 +-
>  security/integrity/integrity.h|  5 ++---
>  7 files changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 73c457b..21d7568 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -41,6 +41,17 @@ config INTEGRITY_ASYMMETRIC_KEYS
> This option enables digital signature verification using
> asymmetric keys.
>  
> +config INTEGRITY_TRUSTED_KEYRING
> + bool "Require all keys on the integrity keyrings be signed"
> + depends on SYSTEM_TRUSTED_KEYRING
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + select KEYS_DEBUG_PROC_KEYS
> + default y
> + help
> +This option requires that all keys added to the .ima and
> +.evm keyrings be signed by a key on the system trusted
> +keyring.
> +
>  config INTEGRITY_AUDIT
>   bool "Enables integrity auditing support "
>   depends on AUDIT
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5be9ffb..8ef1511 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -24,15 +24,22 @@
>  static struct key *keyring[INTEGRITY_KEYRING_MAX];
>  
>  static const char *keyring_name[INTEGRITY_KEYRING_MAX] = {
> +#ifndef CONFIG_INTEGRITY_TRUSTED_KEYRING
>   "_evm",
> - "_module",
> -#ifndef CONFIG_IMA_TRUSTED_KEYRING
>   "_ima",
>  #else
> + ".evm",
>   ".ima",
>  #endif
> + "_module",
>  };
>  
> +#ifdef CONFIG_INTEGRITY_TRUSTED_KEYRING
> +static bool init_keyring __initdata = true;
> +#else
> +static bool init_keyring __initdata;
> +#endif
> +
>  int integrity_digsig_verify(const unsigned int id, const char *sig, int 
> siglen,
>   const char *digest, int digestlen)
>  {
> @@ -68,6 +75,9 @@ int __init integrity_init_keyring(const unsigned int id)
>   const struct cred *cred = current_cred();
>   int err = 0;
>  
> + if (!init_keyring)
> + return 0;
> +
>   keyring[id] = keyring_alloc(keyring_name[id], KUIDT_INIT(0),
>   KGIDT_INIT(0), cred,
>   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> diff --git a/security/integrity/evm/evm_main.c 
> b/security/integrity/evm/evm_main.c
> index 1334e02..75b7e30 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,15 +478,17 @@ static int __init init_evm(void)
>  
>   evm_init_config();
>  
> + error = integrity_init_keyring(INTEGRITY_KEYRING_EVM);
> + if (error)
> + return error;
> +
>   error = evm_init_secfs();
>   if (error < 0) {
>   pr_info("Error registering secfs\n");
> - goto err;
> + return error;
>   }
>  
>   return 0;
> -err:
> - return error;
>  }
>  
>  /*
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index df30334..a292b88 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -123,14 +123,17 @@ config IMA_APPRAISE
> If unsure, say N.
>  
>  config IMA_TRUSTED_KEYRING
> - bool "Require all keys on the .ima keyring be signed"
> + bool "Require all keys on the .ima keyring be signed (deprecated)"
>   depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
>   depends on INTEGRITY_ASYMMETRIC_KEYS
> + select INTEGRITY_TRUSTED_KEYRING
>   default y
>   help
>  This option requires that all keys added to the .ima
>  keyring be signed by a key on the system trusted keyring.
>  
> +This option is deprecated in favor of INTEGRITY_TRUSTED_KEYRING
> +
>  config IMA_LOAD_X509
>   bool "Load X509 certificate onto the '.ima' trusted keyring"
>   depends on IMA_TRUSTED_KEYRING


I guess we may as well remove this switch.  Otherwise somebody have to remember 
to post a patch that does so a few kernel 

Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted

[Petko Manolov]

On 15-10-21 13:02:48, Mimi Zohar wrote:
> On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote:
> > Here's a set of patches that changes how keys are determined to be trusted
> > - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon
> > it.  A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that
> > indicates that only keys with this flag set may be added to that keyring.
> > 
> > Further, any time an X.509 certificate is instantiated without this flag
> > set, the certificate is judged against the contents of the system trusted
> > keyring to determine whether KEY_FLAG_TRUSTED should be set upon it.
> > 
> > With these patches, KEY_FLAG_TRUSTED is removed.  The kernel may add
> > implicitly trusted keys to a trusted-only keyring by asserting
> > KEY_ALLOC_TRUSTED when the key is created, 
> 
> Ok, but only the x509 certificates built into the kernel image should be
> automatically trusted and can be added to a trusted keyring, because the
> kernel itself was signed (and verified).  These certificates extend the
> (UEFI) certificate chain of trust that is rooted in hardware to the OS.
> 
> Other keys that the kernel reads and loads should not automatically be
> trusted (eg. ima_load_x509).  They need to be validated against a
> trusted key.
> 
> > but otherwise the key will only
> > be allowed to be added to the keyring if it can be verified by a key
> > already in that keyring.  The system trusted keyring is not then special in
> > this sense and other trusted keyrings can be set up that are wholly
> > independent of it.
> 
> We already went down this path of "transitive trust" back when we first 
> introduced the concept of trusted keys and keyrings.  Just because a key is 
> on 
> a trusted keyring, doesn't imply that it should be permitted to load other 
> keys on the same trusted keyring.  In the case of IMA-appraisal, the key 
> should only be used to verify the file data signature, not other keys.
> 
> The trusted keys used for verifying other certificates should be stored on a 
> separate keyring, not the target keyring.  Petko's patches define a new IMA 
> keyring named .ima_mok for this purpose.

The concept is not new.  Some embedded applications are multi-tenant and 
typically have uptime measured in years.  The current CA hierarchy model of the 
kernel is somewhat limited in terms of dynamically adding trusted certificates 
and trusted keys.

.ima_mok was introduced as an intermediate keyring storing CAs that are 
themselves signed by CAs in the system keyring, which is trusted by default.  
Only keys that have been signed by certificate in .system or .ima_mok may land 
in .ima keyring.  This:

.system ---> .ima_mok ---> .ima ---> actual.key

gives us the ability to extend the chain of trust and also cover the above 
criteria.  That said, .ima_mok may be used for a whole bunch of other cases.

Think of a kernel module that comes from one of the tenants or even the machine 
owner.  They obviously don't have access to the Manufacturer's signing key 
(CA-M), but do have certificate (CA-O) that has been signed by it (CA-M).

This certificate (CA-O) can now go to .ima_mok (or whatever the name) and 
successfully verify the kernel's module signature.  CA-O may even sign another 
certificate, CA-O2, and by the above rules it may also go into .ima_mok.  And 
so 
on...

I think that in general having an intermediate CA keyring adds a lot of 
flexibility to the kernel's key management, although it's typical use does not 
make this mandatory.


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 00/10] KEYS: Change how keys are determined to be trusted

[Petko Manolov]

On 15-10-21 13:02:48, Mimi Zohar wrote:
> On Wed, 2015-10-21 at 16:13 +0100, David Howells wrote:
> > Here's a set of patches that changes how keys are determined to be trusted
> > - currently, that's a case of whether a key has KEY_FLAG_TRUSTED set upon
> > it.  A keyring can then have a flag set (KEY_FLAG_TRUSTED ONLY) that
> > indicates that only keys with this flag set may be added to that keyring.
> > 
> > Further, any time an X.509 certificate is instantiated without this flag
> > set, the certificate is judged against the contents of the system trusted
> > keyring to determine whether KEY_FLAG_TRUSTED should be set upon it.
> > 
> > With these patches, KEY_FLAG_TRUSTED is removed.  The kernel may add
> > implicitly trusted keys to a trusted-only keyring by asserting
> > KEY_ALLOC_TRUSTED when the key is created, 
> 
> Ok, but only the x509 certificates built into the kernel image should be
> automatically trusted and can be added to a trusted keyring, because the
> kernel itself was signed (and verified).  These certificates extend the
> (UEFI) certificate chain of trust that is rooted in hardware to the OS.
> 
> Other keys that the kernel reads and loads should not automatically be
> trusted (eg. ima_load_x509).  They need to be validated against a
> trusted key.
> 
> > but otherwise the key will only
> > be allowed to be added to the keyring if it can be verified by a key
> > already in that keyring.  The system trusted keyring is not then special in
> > this sense and other trusted keyrings can be set up that are wholly
> > independent of it.
> 
> We already went down this path of "transitive trust" back when we first 
> introduced the concept of trusted keys and keyrings.  Just because a key is 
> on 
> a trusted keyring, doesn't imply that it should be permitted to load other 
> keys on the same trusted keyring.  In the case of IMA-appraisal, the key 
> should only be used to verify the file data signature, not other keys.
> 
> The trusted keys used for verifying other certificates should be stored on a 
> separate keyring, not the target keyring.  Petko's patches define a new IMA 
> keyring named .ima_mok for this purpose.

The concept is not new.  Some embedded applications are multi-tenant and 
typically have uptime measured in years.  The current CA hierarchy model of the 
kernel is somewhat limited in terms of dynamically adding trusted certificates 
and trusted keys.

.ima_mok was introduced as an intermediate keyring storing CAs that are 
themselves signed by CAs in the system keyring, which is trusted by default.  
Only keys that have been signed by certificate in .system or .ima_mok may land 
in .ima keyring.  This:

.system ---> .ima_mok ---> .ima ---> actual.key

gives us the ability to extend the chain of trust and also cover the above 
criteria.  That said, .ima_mok may be used for a whole bunch of other cases.

Think of a kernel module that comes from one of the tenants or even the machine 
owner.  They obviously don't have access to the Manufacturer's signing key 
(CA-M), but do have certificate (CA-O) that has been signed by it (CA-M).

This certificate (CA-O) can now go to .ima_mok (or whatever the name) and 
successfully verify the kernel's module signature.  CA-O may even sign another 
certificate, CA-O2, and by the above rules it may also go into .ima_mok.  And 
so 
on...

I think that in general having an intermediate CA keyring adds a lot of 
flexibility to the kernel's key management, although it's typical use does not 
make this mandatory.


cheers,
Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] X.509: Fix certificate gathering again

[Petko Manolov]

On May 26, 2015 7:15:38 PM GMT+03:00, David Howells  wrote:
>Hi Michal,
>
>Could you have a look at the patch at the end of my branch:
>
>   
> http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7
>
>It changes things from picking up arbitrary *.x509 files dropped in the
>kernel
>source and/or build directory to taking a single named PEM file with
>all the
>additional certs as a string config option.  The PEM file can contain
>multiple
>certs simply cat'd together.
>
>If you're okay with that, it obsoletes these patches of yours.
>
>I've attached it here for convenience also.
>
>David
>---
>commit 9c71c950793b1b8c23c6d945b31f6545f82adced
>Author: David Woodhouse 
>Date:   Thu May 21 12:23:55 2015 +0100
>
>modsign: Add explicit CONFIG_SYSTEM_TRUSTED_KEYS option
>
>Let the user explicitly provide a file containing trusted keys, instead
>of
> just automatically finding files matching *.x509 in the build tree and
>trusting whatever we find. This really ought to be an *explicit*
>configuration, and the build rules for dealing with the files were
>fairly painful too.
>
>Signed-off-by: David Woodhouse 
>Signed-off-by: David Howells 
>
>diff --git a/Documentation/module-signing.txt
>b/Documentation/module-signing.txt
>index 5d5e4e32dc26..4e62bc29666e 100644
>--- a/Documentation/module-signing.txt
>+++ b/Documentation/module-signing.txt
>@@ -88,6 +88,7 @@ This has a number of options available:
>than being a module) so that modules signed with that algorithm can
>have
>  their signatures checked without causing a dependency loop.
> 
>+
>(4) "File name or PKCS#11 URI of module signing key"
>(CONFIG_MODULE_SIG_KEY)
> 
>  Setting this option to something other than its default of
>@@ -104,6 +105,13 @@ This has a number of options available:
>  means of the KBUILD_SIGN_PIN variable.
> 
> 
>+ (5) "Additional X.509 keys for default system keyring"
>(CONFIG_SYSTEM_TRUSTED_KEYS)
>+
>+ This option can be set to the filename of a PEM-encoded file
>containing
>+ additional certificates which will be included in the system
>keyring by
>+ default.
>+
>+
> ===
> GENERATING SIGNING KEYS
> ===
>@@ -171,10 +179,9 @@ in a keyring called ".system_keyring" that can be
>seen by:
>   302d2d52 I-- 1 perm 1f01 0 0 asymmetri Fedora
>kernel signing key: d69a84e6bce3d216b979e9505b3e3ef9a7118079: X509.RSA
>a7118079 []
>   ...
> 
>-Beyond the public key generated specifically for module signing, any
>file
>-placed in the kernel source root directory or the kernel build root
>directory
>-whose name is suffixed with ".x509" will be assumed to be an X.509
>public key
>-and will be added to the keyring.
>+Beyond the public key generated specifically for module signing,

I think this should be "private", not "public" key. The modules are signed with 
the private key...


Petko

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-26 18:08:13, One Thousand Gnomes wrote:
> On Thu, 21 May 2015 18:53:19 +0300
> Petko Manolov  wrote:
> 
> > It is device's job to verify firmware's correctness.  It is user's job to 
> > verify vendor's identity.  Two different things, not related to each other.
> 
> The device verifies the firmwares identity. The firmware's correctness is 
> unknownable if the mathematicians are correct.

Or so the story goes... :)

Anyway, bad choice of words.  I meant to say its device's responsibility to 
verify binary blob's integrity, be it firmware or something else.

> The device will accept firmware signed in some manner with some key that is 
> probably part of a root of trust embedded deeply im the hardware itself. If 
> it's vendor X hardware then firmware not signed with the key for that 
> hardware 
> won't work, and vendor X has the key locked away.

Ideally, this would be the case.  However, as far as i know there is very few 
devices out there that will do what you describe.  First, it is complex.  
Second, and more important, it is expensive.  Third, if the root of trust is 
compromised the device is done for.  Or rather its user.

Considering the above i guess this isn't going to happen anytime soon, except 
for highly specialized devices.  Of course i may not be correct.  In 
mathematical sense as well. :)

> It's also worth remembering most of the dumb non signature checking devices 
> are things like USB. They don't have access to the internals of the system so 
> their attack options are more limited.

Most devices just lack the infrastructure.  Especially those that are used in 
the embedded world.  Very few of them are designed to withstand relatively 
moderate attempt on their security.  Speaking about hardware.  Come the 
software, 
things are quickly getting messier.


> On Thu, 21 May 2015 16:03:02 +
> "Woodhouse, David"  wrote:
> 
> > In the case where kernel and modules are signed, it *is* useful for a 
> > kernel 
> > device driver also to be able to validate that what it's about to load into 
> > a device is authentic.
> 
> You also need to know its "authentic" for that specific device. Otherwise you 
> may be able to exploit something by loading an authentic firmware for another 
> piece of hardware.

Well, yes this may easily happen.  Integrity checks aren't going to help here.

> Ie you need to sign something more than the firmware, such as (firmware, 
> modinfo), so it's signed for "firmware X on PCI:8086,1114 or "firmware Y on 
> ACPI:0A1D"

Should work as long as the HW IDs can't be tampered with.

> I want to understand the model, who signs what, and what security is 
> allegedly 
> provided over the existing. If there are users sufficiently paranoid to 
> believe that signing firmware saves them, then fine. For most hardware it can 
> cut out some attackers, although anyone with sufficient money or a TLA can no 
> doubt just tap someone on the shoulder and say you are signing this for us.

That's always the case with security, root/chain of trust, etc.  There's always 
someone with bigger gun.  Most systems that aren't compromised are those that 
do 
not draw attention, mostly because of their insignificance.

> IMHO we want the supplier of a given firmware providing signatures on the 
> firmware git tree if this is done. A generic linux-firmware owned key would 
> be 
> both a horrendously inviting attack target, and a single point of failure.
>
> Git can already do all the needed commit signing bits unless I'm missing 
> something here ?

If i read the above correctly you propose to have a tree where all binary blobs 
(or whatever data) will be trusted, because their authenticity will be verified 
prior to their inclusion?  By cloning this tree i should also trust its content 
because GIT takes care of data's integrity?


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-26 18:08:13, One Thousand Gnomes wrote:
 On Thu, 21 May 2015 18:53:19 +0300
 Petko Manolov pet...@mip-labs.com wrote:
 
  It is device's job to verify firmware's correctness.  It is user's job to 
  verify vendor's identity.  Two different things, not related to each other.
 
 The device verifies the firmwares identity. The firmware's correctness is 
 unknownable if the mathematicians are correct.

Or so the story goes... :)

Anyway, bad choice of words.  I meant to say its device's responsibility to 
verify binary blob's integrity, be it firmware or something else.

 The device will accept firmware signed in some manner with some key that is 
 probably part of a root of trust embedded deeply im the hardware itself. If 
 it's vendor X hardware then firmware not signed with the key for that 
 hardware 
 won't work, and vendor X has the key locked away.

Ideally, this would be the case.  However, as far as i know there is very few 
devices out there that will do what you describe.  First, it is complex.  
Second, and more important, it is expensive.  Third, if the root of trust is 
compromised the device is done for.  Or rather its user.

Considering the above i guess this isn't going to happen anytime soon, except 
for highly specialized devices.  Of course i may not be correct.  In 
mathematical sense as well. :)

 It's also worth remembering most of the dumb non signature checking devices 
 are things like USB. They don't have access to the internals of the system so 
 their attack options are more limited.

Most devices just lack the infrastructure.  Especially those that are used in 
the embedded world.  Very few of them are designed to withstand relatively 
moderate attempt on their security.  Speaking about hardware.  Come the 
software, 
things are quickly getting messier.


 On Thu, 21 May 2015 16:03:02 +
 Woodhouse, David david.woodho...@intel.com wrote:
 
  In the case where kernel and modules are signed, it *is* useful for a 
  kernel 
  device driver also to be able to validate that what it's about to load into 
  a device is authentic.
 
 You also need to know its authentic for that specific device. Otherwise you 
 may be able to exploit something by loading an authentic firmware for another 
 piece of hardware.

Well, yes this may easily happen.  Integrity checks aren't going to help here.

 Ie you need to sign something more than the firmware, such as (firmware, 
 modinfo), so it's signed for firmware X on PCI:8086,1114 or firmware Y on 
 ACPI:0A1D

Should work as long as the HW IDs can't be tampered with.

 I want to understand the model, who signs what, and what security is 
 allegedly 
 provided over the existing. If there are users sufficiently paranoid to 
 believe that signing firmware saves them, then fine. For most hardware it can 
 cut out some attackers, although anyone with sufficient money or a TLA can no 
 doubt just tap someone on the shoulder and say you are signing this for us.

That's always the case with security, root/chain of trust, etc.  There's always 
someone with bigger gun.  Most systems that aren't compromised are those that 
do 
not draw attention, mostly because of their insignificance.

 IMHO we want the supplier of a given firmware providing signatures on the 
 firmware git tree if this is done. A generic linux-firmware owned key would 
 be 
 both a horrendously inviting attack target, and a single point of failure.

 Git can already do all the needed commit signing bits unless I'm missing 
 something here ?

If i read the above correctly you propose to have a tree where all binary blobs 
(or whatever data) will be trusted, because their authenticity will be verified 
prior to their inclusion?  By cloning this tree i should also trust its content 
because GIT takes care of data's integrity?


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] X.509: Fix certificate gathering again

[Petko Manolov]

On May 26, 2015 7:15:38 PM GMT+03:00, David Howells dhowe...@redhat.com wrote:
Hi Michal,

Could you have a look at the patch at the end of my branch:

   
 http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=modsign-pkcs7

It changes things from picking up arbitrary *.x509 files dropped in the
kernel
source and/or build directory to taking a single named PEM file with
all the
additional certs as a string config option.  The PEM file can contain
multiple
certs simply cat'd together.

If you're okay with that, it obsoletes these patches of yours.

I've attached it here for convenience also.

David
---
commit 9c71c950793b1b8c23c6d945b31f6545f82adced
Author: David Woodhouse david.woodho...@intel.com
Date:   Thu May 21 12:23:55 2015 +0100

modsign: Add explicit CONFIG_SYSTEM_TRUSTED_KEYS option

Let the user explicitly provide a file containing trusted keys, instead
of
 just automatically finding files matching *.x509 in the build tree and
trusting whatever we find. This really ought to be an *explicit*
configuration, and the build rules for dealing with the files were
fairly painful too.

Signed-off-by: David Woodhouse david.woodho...@intel.com
Signed-off-by: David Howells dhowe...@redhat.com

diff --git a/Documentation/module-signing.txt
b/Documentation/module-signing.txt
index 5d5e4e32dc26..4e62bc29666e 100644
--- a/Documentation/module-signing.txt
+++ b/Documentation/module-signing.txt
@@ -88,6 +88,7 @@ This has a number of options available:
than being a module) so that modules signed with that algorithm can
have
  their signatures checked without causing a dependency loop.
 
+
(4) File name or PKCS#11 URI of module signing key
(CONFIG_MODULE_SIG_KEY)
 
  Setting this option to something other than its default of
@@ -104,6 +105,13 @@ This has a number of options available:
  means of the KBUILD_SIGN_PIN variable.
 
 
+ (5) Additional X.509 keys for default system keyring
(CONFIG_SYSTEM_TRUSTED_KEYS)
+
+ This option can be set to the filename of a PEM-encoded file
containing
+ additional certificates which will be included in the system
keyring by
+ default.
+
+
 ===
 GENERATING SIGNING KEYS
 ===
@@ -171,10 +179,9 @@ in a keyring called .system_keyring that can be
seen by:
   302d2d52 I-- 1 perm 1f01 0 0 asymmetri Fedora
kernel signing key: d69a84e6bce3d216b979e9505b3e3ef9a7118079: X509.RSA
a7118079 []
   ...
 
-Beyond the public key generated specifically for module signing, any
file
-placed in the kernel source root directory or the kernel build root
directory
-whose name is suffixed with .x509 will be assumed to be an X.509
public key
-and will be added to the keyring.
+Beyond the public key generated specifically for module signing,

I think this should be private, not public key. The modules are signed with 
the private key...


Petko

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-22 08:28:17, Mimi Zohar wrote:
> On Fri, 2015-05-22 at 08:48 +0100, David Howells wrote:
> > Luis R. Rodriguez  wrote:
> > 
> > > > This is similar to what i am doing right now - create CA hierarchy so 
> > > > we can 
> > > > have something like:
> > > > 
> > > >+-> KeyB
> > > >|
> > > > RootCA --->  CertA ---> CertB ---> CertC ---> KeyC
> > > > |
> > > > +-> CertA' ---> KeyA"
> > > 
> > > How exactly do you go about uploading CertB to the kernel BTW?
> > 
> > Assuming RootCA or CertA is present in the kernel, the idea would be to use 
> > the add_key() system call or the request_key() mechanism to add the key to 
> > the system keyring.  The key in the cert would only be added to the keyring 
> > if it is trusted by a key already there.
> 
> From Petko's description, the RootCA is on the system keyring, but CertA is 
> on 
> a new IMA trusted CA keyring.  So everything you said is true, but on this 
> new, yet to be upstreamed, IMA trusted CA keyring.

I only named this intermediate keyring .ima_root_ca because this is how i use 
it.  However, it is system-wide trusted keyring, which accepts keys that are 
either trusted by CAs in the .system_keyring or higher level CA is already in 
.ima_root_ca.  For example CertC will be accepted in .ima_root_ca if CertB is 
already there.

The name (.ima_root_ca) is misleading and should be replaced with something 
that 
better describes it's functionality.  As far as i see there is no reason this 
keyring not to hold the CA that verifies module's signature.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-22 08:28:17, Mimi Zohar wrote:
 On Fri, 2015-05-22 at 08:48 +0100, David Howells wrote:
  Luis R. Rodriguez mcg...@suse.com wrote:
  
This is similar to what i am doing right now - create CA hierarchy so 
we can 
have something like:

   +- KeyB
   |
RootCA ---  CertA --- CertB --- CertC --- KeyC
|
+- CertA' --- KeyA
   
   How exactly do you go about uploading CertB to the kernel BTW?
  
  Assuming RootCA or CertA is present in the kernel, the idea would be to use 
  the add_key() system call or the request_key() mechanism to add the key to 
  the system keyring.  The key in the cert would only be added to the keyring 
  if it is trusted by a key already there.
 
 From Petko's description, the RootCA is on the system keyring, but CertA is 
 on 
 a new IMA trusted CA keyring.  So everything you said is true, but on this 
 new, yet to be upstreamed, IMA trusted CA keyring.

I only named this intermediate keyring .ima_root_ca because this is how i use 
it.  However, it is system-wide trusted keyring, which accepts keys that are 
either trusted by CAs in the .system_keyring or higher level CA is already in 
.ima_root_ca.  For example CertC will be accepted in .ima_root_ca if CertB is 
already there.

The name (.ima_root_ca) is misleading and should be replaced with something 
that 
better describes it's functionality.  As far as i see there is no reason this 
keyring not to hold the CA that verifies module's signature.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:55:42, Andy Lutomirski wrote:
> 
> I read plenty of the code.  I said "data" not "file" for a reason.
> I'll quote some code:
> 
> int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
>  struct file *file, const unsigned char *filename,
>  struct evm_ima_xattr_data *xattr_value,
>  int xattr_len, int opened)
> 
> There is no struct file in init_module.
> 
> {
> static const char op[] = "appraise_data";
> char *cause = "unknown";
> struct dentry *dentry = file->f_path.dentry;
> struct inode *inode = dentry->d_inode;
> enum integrity_status status = INTEGRITY_UNKNOWN;
> int rc = xattr_len, hash_start = 0;
> 
> if (!inode->i_op->getxattr)
> return INTEGRITY_UNKNOWN;

Yes, not all filesystems support extended attributes, but this is not 
necessarily end of the game.  IMA code may be modified to use detached 
signatures.  In fact i was considering this option not so long ago, but dropped 
the idea as more elegant solution was presented.

> I maintain my claim that IMA is not appropriate for module signing in 
> general.  
> It might make sense for the kind of thing that Chromium does (approving of 
> modules using finit_module based on their source instead of their payload and 
> verifying the payload indirectly using IMA or dm-verity), but that's not the 
> problem that David and Luis are trying to solve.

I guess this is all backwards.  Module signing/verification must be autonomous 
process/infrastructure.  The fact that IMA may do the same for you given a few 
conditions are met does not make it a replacement.

I do not use module signing for one particular project, because we're building 
custom kernel and initramfs.  IMA is enabled early on so an attempt to read .ko 
file results in verifying it's signature.  Yes, the filesystem does support 
xattr. :)

> Also, especially for firmware on regular distros, IMA is ridiculous. IIRC 
> there is no general support for xattrs in initramfs, and there is no reason 
> to 
> start requiring such support just to allow firmware to live in initramfs.

For regular distro, maybe.  If one needs better overall security - not so - i 
actually quite like IMA. ;) Adding xattrs to initramfs is not bad idea at all, 
IMA arguments aside.

> I think that using IMA for this has a similar problem to using PKCS#7: it's a 
> big hammer that is much more complex than necessary to solve the problem at 
> hand.

Absolutely.  Using IMA just for module signing is a bit of an overkill.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 10:02:36, gre...@linuxfoundation.org wrote:
> On Thu, May 21, 2015 at 04:03:02PM +, Woodhouse, David wrote:
> > 
> > In a lot of cases we have loadable firmware precisely to allow us to
> > reduce the cost of the hardware. Adding cryptographic capability in the
> > 'load firmware' state of the device isn't really compatible with that
> > :)
> 
> We do?  What devices want this?  That's really a bad hardware design to trust 
> the kernel to get all of this correct.

Which means nearly all hardware we use today is badly designed... :)

> And I say this as someone who is currently working on a hardware design that 
> does just this for a very tiny device.  It's only a few hundred bytes of 
> firmware size to be able to do proper key verification that the firmware 
> image 
> is correct and can be "trusted".

And a "few" more bytes for the hash algorithm along the one for asymmetric key 
computation and management. :)

> > In the case where kernel and modules are signed, it *is* useful for a 
> > kernel 
> > device driver also to be able to validate that what it's about to load into 
> > a device is authentic. Where 'authentic' will originally just mean that 
> > it's 
> > come from the linux-firmware.git repository or the same entity that built 
> > (and signed) the kernel, but actually I *do* expect vendors who are 
> > actively 
> > maintaining the firmware images in linux-firmware.git to start providing 
> > detached signatures of their own.
> 
> Again, why have a detached signature and not just part of the firmware blob?  
> The device needs to be caring about this, not the kernel.

In ideal world this is what should be done.  However, adding the simplest (read 
slowest) MD5 implementation requires a few K's of ram on 32bit cpu.  MD5 is 
dead.  So we need SHA-something, which isn't smaller in terms of code size.  
Add 
the asymmetric cryptography to the picture and we've already put away all 
vendors.

> As the kernel doesn't know/care about what the firmware blob really is, I 
> don't see why it should be caring about firmware signing as that's a binary 
> running on a separate "computer".  Do we want to take this the next logical 
> step further and start requiring networked devices to attest their kernels 
> are 
> signed correctly before we can talk to them?

I think it is enough for you to know that your iwlwifi's firmware comes from 
Intel and not from a random Internet punk.  If you trust Intel with your wifi 
adapter you probably trust them to write good firmware for it.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:48:10, Andy Lutomirski wrote:
> On Thu, May 21, 2015 at 9:43 AM, Petko Manolov  wrote:
> > On 15-05-21 16:51:49, David Howells wrote:
> >>
> >> I do have patches to parse PGP key data and add the public keys found 
> >> therein
> >> onto the kernel keyring, but that would mean adding an extra key data 
> >> parser.
> >
> > PGP is widely used so i would gladly have one more parser in the kernel.
> 
> 
> PGP is also a crappy format:
> 
> http://blog.cryptographyengineering.com/2014/08/whats-matter-with-pgp.html
> 
> and using PGP means we're probably stuck with PKCS#1 v1.5, which should have 
> been phased out worldwide many years ago.
> 
> In any event, I don't see why PGP compatibility requires any sort of
> OpenPGP parser in the kernel.  Just because GnuPG goes out of its way
> to be incompatible with anything other than the OpenPGP ecosystem
> doesn't mean that you can't relatively straightforwardly generate raw
> PKCS#1 signatures from an OpenPGP key.
> 

Oh, i do agree with you in terms of quality of both formats.  However, this is 
what is commonly used these days and not having these parsers in the kernel 
would require us to write tools for conversion to saner format.  Which does not 
exist for the moment.  Once we have those in place it would be all the same to 
me.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:39:50, Andy Lutomirski wrote:
> 
> It's also a performance cost because the average user of this signature stuff 
> doesn't actually want IMA, and IMA is checking the wrong think anyway.  
> IMA/EVM tells us "this file validly belongs in /lib/modules/whatever 
> according 
> to whomever we trust for the filesystem".  We want to check "is this data, 
> regardless of where it was read from, a trusted module".

IMA-appraise does not care where the file comes from (although it may be 
persuaded to) and verifies file's data and meta-data against a signature.  I 
guess you should actually read the code. :)


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 16:51:49, David Howells wrote:
> 
> I do have patches to parse PGP key data and add the public keys found therein
> onto the kernel keyring, but that would mean adding an extra key data parser.

PGP is widely used so i would gladly have one more parser in the kernel.

> You could probably do this with the integrity functions - but turning them on 
> has a performance cost and you have to load things in the right order as I 
> understand it.

The performance hit is negligible, especially on modern hardware.  The problem 
is that Joe user must wrap his head around IMA as a concept and go through the 
pains of doing everything right.  Failing to do so will result in a lot of 
frustration, and i speak from experience.

Once you make it run properly it mostly stays out of your way.  To put it 
another way: IMA is not for sissies... :)


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 08:45:08, Greg Kroah-Hartman wrote:
> On Thu, May 21, 2015 at 09:05:21AM -0400, Mimi Zohar wrote:
> > 
> > Signatures don't provide any guarantees as to code quality or
> > correctness.   They do provide file integrity and provenance.  In
> > addition to the license and a Signed-off-by line, having the firmware
> > provider include a signature of the firmware would be nice.
> 
> That would be "nice", but that's not going to be happening here, from what I 
> can tell.  The firmware provider should be putting the signature inside the 
> firmware image itself, and verifying it on the device, in order to properly 
> "know" that it should be running that firmware.  The kernel shouldn't be 
> involved here at all, as Alan pointed out.

It is device's job to verify firmware's correctness.  It is user's job to 
verify 
vendor's identity.  Two different things, not related to each other.

I think Alan meant something else.  What i read is that if somebody have 
physical access to the device they may harm the device much easier and would 
not bother to tamper with firmware.

> > > What is verifying a firmware image signature in the kernel attesting
> > > that isn't already known in userspace?
> > 
> > Appraising and enforcing firmware integrity before use.
> 
> That should be done on the device itself, not in the kernel.

Oh, well...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 23:14:53, Greg Kroah-Hartman wrote:
> 
> Ok, but how do we know "where"?  Who is going to start signing and attesting 
> to the validity of all of the firmware images in the linux-firmware tree 
> suddenly?  Why is it the kernel's job to attest this "where"?  Shouldn't your 
> distro/manufacturer be doing that as part of their "put this file on this 
> disk" responsibilities (i.e. the package manager?)

I did not say the kernel should care about signatures.  This is entirely user's
choice.  You care about authenticity, you require signature and verify it
against "known good" certificate. s/you/distro/

I'm just saying that it would be nice to have this feature in case somebody 
need 
it.  Luckily IMA has this functionality, although you'll have to work hard to 
get to use it. :)

> What is verifying a firmware image signature in the kernel attesting that 
> isn't already known in userspace?

The kernel is a lot better place to keep certificates and keys safe, compared 
to 
userland.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:55:42, Andy Lutomirski wrote:
 
 I read plenty of the code.  I said data not file for a reason.
 I'll quote some code:
 
 int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
  struct file *file, const unsigned char *filename,
  struct evm_ima_xattr_data *xattr_value,
  int xattr_len, int opened)
 
 There is no struct file in init_module.
 
 {
 static const char op[] = appraise_data;
 char *cause = unknown;
 struct dentry *dentry = file-f_path.dentry;
 struct inode *inode = dentry-d_inode;
 enum integrity_status status = INTEGRITY_UNKNOWN;
 int rc = xattr_len, hash_start = 0;
 
 if (!inode-i_op-getxattr)
 return INTEGRITY_UNKNOWN;

Yes, not all filesystems support extended attributes, but this is not 
necessarily end of the game.  IMA code may be modified to use detached 
signatures.  In fact i was considering this option not so long ago, but dropped 
the idea as more elegant solution was presented.

 I maintain my claim that IMA is not appropriate for module signing in 
 general.  
 It might make sense for the kind of thing that Chromium does (approving of 
 modules using finit_module based on their source instead of their payload and 
 verifying the payload indirectly using IMA or dm-verity), but that's not the 
 problem that David and Luis are trying to solve.

I guess this is all backwards.  Module signing/verification must be autonomous 
process/infrastructure.  The fact that IMA may do the same for you given a few 
conditions are met does not make it a replacement.

I do not use module signing for one particular project, because we're building 
custom kernel and initramfs.  IMA is enabled early on so an attempt to read .ko 
file results in verifying it's signature.  Yes, the filesystem does support 
xattr. :)

 Also, especially for firmware on regular distros, IMA is ridiculous. IIRC 
 there is no general support for xattrs in initramfs, and there is no reason 
 to 
 start requiring such support just to allow firmware to live in initramfs.

For regular distro, maybe.  If one needs better overall security - not so - i 
actually quite like IMA. ;) Adding xattrs to initramfs is not bad idea at all, 
IMA arguments aside.

 I think that using IMA for this has a similar problem to using PKCS#7: it's a 
 big hammer that is much more complex than necessary to solve the problem at 
 hand.

Absolutely.  Using IMA just for module signing is a bit of an overkill.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 23:14:53, Greg Kroah-Hartman wrote:
 
 Ok, but how do we know where?  Who is going to start signing and attesting 
 to the validity of all of the firmware images in the linux-firmware tree 
 suddenly?  Why is it the kernel's job to attest this where?  Shouldn't your 
 distro/manufacturer be doing that as part of their put this file on this 
 disk responsibilities (i.e. the package manager?)

I did not say the kernel should care about signatures.  This is entirely user's
choice.  You care about authenticity, you require signature and verify it
against known good certificate. s/you/distro/

I'm just saying that it would be nice to have this feature in case somebody 
need 
it.  Luckily IMA has this functionality, although you'll have to work hard to 
get to use it. :)

 What is verifying a firmware image signature in the kernel attesting that 
 isn't already known in userspace?

The kernel is a lot better place to keep certificates and keys safe, compared 
to 
userland.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 08:45:08, Greg Kroah-Hartman wrote:
 On Thu, May 21, 2015 at 09:05:21AM -0400, Mimi Zohar wrote:
  
  Signatures don't provide any guarantees as to code quality or
  correctness.   They do provide file integrity and provenance.  In
  addition to the license and a Signed-off-by line, having the firmware
  provider include a signature of the firmware would be nice.
 
 That would be nice, but that's not going to be happening here, from what I 
 can tell.  The firmware provider should be putting the signature inside the 
 firmware image itself, and verifying it on the device, in order to properly 
 know that it should be running that firmware.  The kernel shouldn't be 
 involved here at all, as Alan pointed out.

It is device's job to verify firmware's correctness.  It is user's job to 
verify 
vendor's identity.  Two different things, not related to each other.

I think Alan meant something else.  What i read is that if somebody have 
physical access to the device they may harm the device much easier and would 
not bother to tamper with firmware.

   What is verifying a firmware image signature in the kernel attesting
   that isn't already known in userspace?
  
  Appraising and enforcing firmware integrity before use.
 
 That should be done on the device itself, not in the kernel.

Oh, well...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:39:50, Andy Lutomirski wrote:
 
 It's also a performance cost because the average user of this signature stuff 
 doesn't actually want IMA, and IMA is checking the wrong think anyway.  
 IMA/EVM tells us this file validly belongs in /lib/modules/whatever 
 according 
 to whomever we trust for the filesystem.  We want to check is this data, 
 regardless of where it was read from, a trusted module.

IMA-appraise does not care where the file comes from (although it may be 
persuaded to) and verifies file's data and meta-data against a signature.  I 
guess you should actually read the code. :)


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 09:48:10, Andy Lutomirski wrote:
 On Thu, May 21, 2015 at 9:43 AM, Petko Manolov pet...@mip-labs.com wrote:
  On 15-05-21 16:51:49, David Howells wrote:
 
  I do have patches to parse PGP key data and add the public keys found 
  therein
  onto the kernel keyring, but that would mean adding an extra key data 
  parser.
 
  PGP is widely used so i would gladly have one more parser in the kernel.
 
 rant
 PGP is also a crappy format:
 
 http://blog.cryptographyengineering.com/2014/08/whats-matter-with-pgp.html
 
 and using PGP means we're probably stuck with PKCS#1 v1.5, which should have 
 been phased out worldwide many years ago.
 
 In any event, I don't see why PGP compatibility requires any sort of
 OpenPGP parser in the kernel.  Just because GnuPG goes out of its way
 to be incompatible with anything other than the OpenPGP ecosystem
 doesn't mean that you can't relatively straightforwardly generate raw
 PKCS#1 signatures from an OpenPGP key.
 /rant

Oh, i do agree with you in terms of quality of both formats.  However, this is 
what is commonly used these days and not having these parsers in the kernel 
would require us to write tools for conversion to saner format.  Which does not 
exist for the moment.  Once we have those in place it would be all the same to 
me.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 16:51:49, David Howells wrote:
 
 I do have patches to parse PGP key data and add the public keys found therein
 onto the kernel keyring, but that would mean adding an extra key data parser.

PGP is widely used so i would gladly have one more parser in the kernel.

 You could probably do this with the integrity functions - but turning them on 
 has a performance cost and you have to load things in the right order as I 
 understand it.

The performance hit is negligible, especially on modern hardware.  The problem 
is that Joe user must wrap his head around IMA as a concept and go through the 
pains of doing everything right.  Failing to do so will result in a lot of 
frustration, and i speak from experience.

Once you make it run properly it mostly stays out of your way.  To put it 
another way: IMA is not for sissies... :)


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-21 10:02:36, gre...@linuxfoundation.org wrote:
 On Thu, May 21, 2015 at 04:03:02PM +, Woodhouse, David wrote:
  
  In a lot of cases we have loadable firmware precisely to allow us to
  reduce the cost of the hardware. Adding cryptographic capability in the
  'load firmware' state of the device isn't really compatible with that
  :)
 
 We do?  What devices want this?  That's really a bad hardware design to trust 
 the kernel to get all of this correct.

Which means nearly all hardware we use today is badly designed... :)

 And I say this as someone who is currently working on a hardware design that 
 does just this for a very tiny device.  It's only a few hundred bytes of 
 firmware size to be able to do proper key verification that the firmware 
 image 
 is correct and can be trusted.

And a few more bytes for the hash algorithm along the one for asymmetric key 
computation and management. :)

  In the case where kernel and modules are signed, it *is* useful for a 
  kernel 
  device driver also to be able to validate that what it's about to load into 
  a device is authentic. Where 'authentic' will originally just mean that 
  it's 
  come from the linux-firmware.git repository or the same entity that built 
  (and signed) the kernel, but actually I *do* expect vendors who are 
  actively 
  maintaining the firmware images in linux-firmware.git to start providing 
  detached signatures of their own.
 
 Again, why have a detached signature and not just part of the firmware blob?  
 The device needs to be caring about this, not the kernel.

In ideal world this is what should be done.  However, adding the simplest (read 
slowest) MD5 implementation requires a few K's of ram on 32bit cpu.  MD5 is 
dead.  So we need SHA-something, which isn't smaller in terms of code size.  
Add 
the asymmetric cryptography to the picture and we've already put away all 
vendors.

 As the kernel doesn't know/care about what the firmware blob really is, I 
 don't see why it should be caring about firmware signing as that's a binary 
 running on a separate computer.  Do we want to take this the next logical 
 step further and start requiring networked devices to attest their kernels 
 are 
 signed correctly before we can talk to them?

I think it is enough for you to know that your iwlwifi's firmware comes from 
Intel and not from a random Internet punk.  If you trust Intel with your wifi 
adapter you probably trust them to write good firmware for it.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 21:41:04, Greg Kroah-Hartman wrote:
> On Wed, May 20, 2015 at 07:46:13PM +0300, Petko Manolov wrote:
> > On 15-05-20 17:24:46, One Thousand Gnomes wrote:
> > > 
> > > More to the point why do you want to sign firmware files ? Leaving aside 
> > > the 
> > > fact that someone will produce a device with GPLv3 firmware just to p*ss 
> > > you 
> > > off there's the rather more relevant fact that firmware for devices on a 
> > > so 
> > > called "trusted" platform already have signed firmware.
> > 
> > For "trusted" systems one would like to make sure everything that goes in 
> > has 
> > known provenance.  Maybe this was the idea?
> 
> If so, then just do what people do today, verify their known valid disk image 
> before mounting it and then they know they can trust the data on it to be use 
> for whatever (including firmware.)  No kernel changes needed, distro support 
> is already there for this.

I do agree, the infrastructure is already in place.  The project i am working 
on 
has very strict security requirements, quite unlike regular Linux box.  I was 
pleasantly surprised that it didn't take much kernel hacking to get to the 
point 
where stuff is working to our liking.

> I too don't understand this need to sign something that you don't really know 
> what it is from some other company, just to send it to a separate device that 
> is going to do whatever it wants with it if it is signed or not.

This is not the point.  What you need to know is _where_ the firmware came 
from, 
not _what_ it does once it reach your system.  If you don't care about such 
things, just ignore the signature. :)


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-20 09:41:21, Andy Lutomirski wrote:
> On Wed, May 20, 2015 at 9:21 AM, Petko Manolov  wrote:
> > On 15-05-20 08:56:21, Andy Lutomirski wrote:
> >>
> >> Would it make more sense to permit X.509 chains to be loaded into the 
> >> keyring
> >> instead if we actually need that feature?  IOW, let userspace (or early
> >> initramfs stuff) extend our keyring trust to intermediate certs that 
> >> validly
> >> chain to already-trusted things?  I think that a reasonable design goal 
> >> would
> >> be that everything overcomplicated that's involved should be optional, and
> >> moving toward embedding PKCS#7 signatures in the modules themselves does 
> >> the
> >> other direction?
> >
> > This is similar to what i am doing right now - create CA hierarchy so we can
> > have something like:
> >
> >+-> KeyB
> >|
> > RootCA --->  CertA ---> CertB ---> CertC ---> KeyC
> > |
> > +-> CertA' ---> KeyA"
> >
> > The RootCA may be the one whose private key was used to sign the modules 
> > and all
> > downstream certificates are either directly signed by it or one of the 
> > others.
> > Not all of the infrastructure is in the mainline kernel, but this can 
> > easily be
> > rectified.
> 
> Right.  I guess that I can imagine some uses for this, but I don't see
> why those intermediate certs would be embedded with the signatures
> being verified as opposed to being loaded beforehand.

They aren't.  They shouldn't.

I think module signing/verification should be simple but cryptographically 
sound 
process.  Over-designing it is dumb, but i find it useful when there is 
compatibility (at least at key format level) with other systems, namely IMA and 
EVM.

> > Now, as Mimi pointed out this scheme is flawed and should be used with care 
> > if at all.  Revoking certificates is always a PITA.  Being valid for one 
> > year only adds to the fun.
> >
> 
> Valid for only one year is worse than that.  We might be verifying the 
> signature on our clock driver :) I think that, at best, we could reject 
> certificates that expired before the running kernel was built.

Nah, nothing as complex as that.  It is so easy to play with the system clock.  
In the general case, and if needed, the user would either use self signed 
certificate or one signed by whoever the user trusts - Fedora, Debian, LF or 
even NSA.  :)


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 17:24:46, One Thousand Gnomes wrote:
> 
> More to the point why do you want to sign firmware files ? Leaving aside the 
> fact that someone will produce a device with GPLv3 firmware just to p*ss you 
> off there's the rather more relevant fact that firmware for devices on a so 
> called "trusted" platform already have signed firmware.

For "trusted" systems one would like to make sure everything that goes in has 
known provenance.  Maybe this was the idea?

> For external devices I don't normally have access to read system memory 
> anyway, and signing firmware would achieve nothing unless you start doing 
> crazy DRM style key exchanges to prove the endpoint is trusted. Any NSA 
> trojan 
> wifi stick is simply going to nod as the correct firmware is uploaded, and 
> then ignore it. And if I'm just out to be a pain I can already just plug in a 
> fake device claiming to be a usb disk with 256 bytes per sector (boom... exit 
> machine stage right), or for that matter wire a USB stick with 5v connected 
> to 
> the mains at the nearest wall socket.

Yep, gaining physical access to the system is a game over.  It is arguable how 
"trusted" a networked machine could be and i guess the answer is "not much"...


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-20 08:56:21, Andy Lutomirski wrote:
> 
> Would it make more sense to permit X.509 chains to be loaded into the keyring 
> instead if we actually need that feature?  IOW, let userspace (or early 
> initramfs stuff) extend our keyring trust to intermediate certs that validly 
> chain to already-trusted things?  I think that a reasonable design goal would 
> be that everything overcomplicated that's involved should be optional, and 
> moving toward embedding PKCS#7 signatures in the modules themselves does the 
> other direction?

This is similar to what i am doing right now - create CA hierarchy so we can 
have something like:

   +-> KeyB
   |
RootCA --->  CertA ---> CertB ---> CertC ---> KeyC
|
+-> CertA' ---> KeyA"

The RootCA may be the one whose private key was used to sign the modules and 
all 
downstream certificates are either directly signed by it or one of the others.  
Not all of the infrastructure is in the mainline kernel, but this can easily be 
rectified.

Now, as Mimi pointed out this scheme is flawed and should be used with care if 
at all.  Revoking certificates is always a PITA.  Being valid for one year only 
adds to the fun.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-19 17:22:59, Luis R. Rodriguez wrote:
>
> I have a series of reasons find IMA unsuitable for the current goals at hand:
> 
>   1) IMA is a pretty big kitchen sink, we want this to work well for
> even embedded systems, or architectures that do not have or require
> TPMs

No, it isn't.  I've profiled it and performance hit is negligible.  All hash 
algorithms used have been optimized for most cpu architectures.

>   2) The appraisal is also done for to account for a specific state of
> affairs, you appraise to the user of the integrity of the system at a
> specific point in time, firmware signing can provide integrity /
> authorship vetting of files directly from the authors. In the case of
> regulatory.bin that was the whole point of it, and firmware signing as
> is being provided is intended to generalize that but by sharing code
> in-kernel with module signing infrastructure

This is weird English to me, but since i am no native speaker either i'll blame 
myself. :)  Could you please rephrase?

> If we go with IMA, I however do not think this would be appropriate and 
> overkill at this point in time.

Depends on what your needs are.  If you need authenticity then IMA-appraise is 
definitely your way to go.  For anything less secure you may go with LSM of 
choice to apply whatever policy you may have in mind.

Again, security and convenience do not play well together.


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-19 19:10:07, Andy Lutomirski wrote:
> On Tue, May 19, 2015 at 7:05 PM, Mimi Zohar  wrote:
> >
> > Perhaps you're referring to EVM?
> 
> I don't know.  I also couldn't figure out what IMA was and what EVM was.

Try reading this:

http://sourceforge.net/p/linux-ima/wiki/Home/

A little outdated with some bits missing, but definitely worth reading.  It 
will 
also help lower the noise in LSM mailing list. :)


Petko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-20 08:56:21, Andy Lutomirski wrote:
 
 Would it make more sense to permit X.509 chains to be loaded into the keyring 
 instead if we actually need that feature?  IOW, let userspace (or early 
 initramfs stuff) extend our keyring trust to intermediate certs that validly 
 chain to already-trusted things?  I think that a reasonable design goal would 
 be that everything overcomplicated that's involved should be optional, and 
 moving toward embedding PKCS#7 signatures in the modules themselves does the 
 other direction?

This is similar to what i am doing right now - create CA hierarchy so we can 
have something like:

   +- KeyB
   |
RootCA ---  CertA --- CertB --- CertC --- KeyC
|
+- CertA' --- KeyA

The RootCA may be the one whose private key was used to sign the modules and 
all 
downstream certificates are either directly signed by it or one of the others.  
Not all of the infrastructure is in the mainline kernel, but this can easily be 
rectified.

Now, as Mimi pointed out this scheme is flawed and should be used with care if 
at all.  Revoking certificates is always a PITA.  Being valid for one year only 
adds to the fun.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 17:24:46, One Thousand Gnomes wrote:
 
 More to the point why do you want to sign firmware files ? Leaving aside the 
 fact that someone will produce a device with GPLv3 firmware just to p*ss you 
 off there's the rather more relevant fact that firmware for devices on a so 
 called trusted platform already have signed firmware.

For trusted systems one would like to make sure everything that goes in has 
known provenance.  Maybe this was the idea?

 For external devices I don't normally have access to read system memory 
 anyway, and signing firmware would achieve nothing unless you start doing 
 crazy DRM style key exchanges to prove the endpoint is trusted. Any NSA 
 trojan 
 wifi stick is simply going to nod as the correct firmware is uploaded, and 
 then ignore it. And if I'm just out to be a pain I can already just plug in a 
 fake device claiming to be a usb disk with 256 bytes per sector (boom... exit 
 machine stage right), or for that matter wire a USB stick with 5v connected 
 to 
 the mains at the nearest wall socket.

Yep, gaining physical access to the system is a game over.  It is arguable how 
trusted a networked machine could be and i guess the answer is not much...


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-19 19:10:07, Andy Lutomirski wrote:
 On Tue, May 19, 2015 at 7:05 PM, Mimi Zohar zo...@linux.vnet.ibm.com wrote:
 
  Perhaps you're referring to EVM?
 
 I don't know.  I also couldn't figure out what IMA was and what EVM was.

Try reading this:

http://sourceforge.net/p/linux-ima/wiki/Home/

A little outdated with some bits missing, but definitely worth reading.  It 
will 
also help lower the noise in LSM mailing list. :)


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/8] MODSIGN: Use PKCS#7 for module signatures [ver #4]

[Petko Manolov]

On 15-05-20 09:41:21, Andy Lutomirski wrote:
 On Wed, May 20, 2015 at 9:21 AM, Petko Manolov pet...@mip-labs.com wrote:
  On 15-05-20 08:56:21, Andy Lutomirski wrote:
 
  Would it make more sense to permit X.509 chains to be loaded into the 
  keyring
  instead if we actually need that feature?  IOW, let userspace (or early
  initramfs stuff) extend our keyring trust to intermediate certs that 
  validly
  chain to already-trusted things?  I think that a reasonable design goal 
  would
  be that everything overcomplicated that's involved should be optional, and
  moving toward embedding PKCS#7 signatures in the modules themselves does 
  the
  other direction?
 
  This is similar to what i am doing right now - create CA hierarchy so we can
  have something like:
 
 +- KeyB
 |
  RootCA ---  CertA --- CertB --- CertC --- KeyC
  |
  +- CertA' --- KeyA
 
  The RootCA may be the one whose private key was used to sign the modules 
  and all
  downstream certificates are either directly signed by it or one of the 
  others.
  Not all of the infrastructure is in the mainline kernel, but this can 
  easily be
  rectified.
 
 Right.  I guess that I can imagine some uses for this, but I don't see
 why those intermediate certs would be embedded with the signatures
 being verified as opposed to being loaded beforehand.

They aren't.  They shouldn't.

I think module signing/verification should be simple but cryptographically 
sound 
process.  Over-designing it is dumb, but i find it useful when there is 
compatibility (at least at key format level) with other systems, namely IMA and 
EVM.

  Now, as Mimi pointed out this scheme is flawed and should be used with care 
  if at all.  Revoking certificates is always a PITA.  Being valid for one 
  year only adds to the fun.
 
 
 Valid for only one year is worse than that.  We might be verifying the 
 signature on our clock driver :) I think that, at best, we could reject 
 certificates that expired before the running kernel was built.

Nah, nothing as complex as that.  It is so easy to play with the system clock.  
In the general case, and if needed, the user would either use self signed 
certificate or one signed by whoever the user trusts - Fedora, Debian, LF or 
even NSA.  :)


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-19 17:22:59, Luis R. Rodriguez wrote:

 I have a series of reasons find IMA unsuitable for the current goals at hand:
 
   1) IMA is a pretty big kitchen sink, we want this to work well for
 even embedded systems, or architectures that do not have or require
 TPMs

No, it isn't.  I've profiled it and performance hit is negligible.  All hash 
algorithms used have been optimized for most cpu architectures.

   2) The appraisal is also done for to account for a specific state of
 affairs, you appraise to the user of the integrity of the system at a
 specific point in time, firmware signing can provide integrity /
 authorship vetting of files directly from the authors. In the case of
 regulatory.bin that was the whole point of it, and firmware signing as
 is being provided is intended to generalize that but by sharing code
 in-kernel with module signing infrastructure

This is weird English to me, but since i am no native speaker either i'll blame 
myself. :)  Could you please rephrase?

 If we go with IMA, I however do not think this would be appropriate and 
 overkill at this point in time.

Depends on what your needs are.  If you need authenticity then IMA-appraise is 
definitely your way to go.  For anything less secure you may go with LSM of 
choice to apply whatever policy you may have in mind.

Again, security and convenience do not play well together.


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFD] linux-firmware key arrangement for firmware signing

[Petko Manolov]

On 15-05-20 21:41:04, Greg Kroah-Hartman wrote:
 On Wed, May 20, 2015 at 07:46:13PM +0300, Petko Manolov wrote:
  On 15-05-20 17:24:46, One Thousand Gnomes wrote:
   
   More to the point why do you want to sign firmware files ? Leaving aside 
   the 
   fact that someone will produce a device with GPLv3 firmware just to p*ss 
   you 
   off there's the rather more relevant fact that firmware for devices on a 
   so 
   called trusted platform already have signed firmware.
  
  For trusted systems one would like to make sure everything that goes in 
  has 
  known provenance.  Maybe this was the idea?
 
 If so, then just do what people do today, verify their known valid disk image 
 before mounting it and then they know they can trust the data on it to be use 
 for whatever (including firmware.)  No kernel changes needed, distro support 
 is already there for this.

I do agree, the infrastructure is already in place.  The project i am working 
on 
has very strict security requirements, quite unlike regular Linux box.  I was 
pleasantly surprised that it didn't take much kernel hacking to get to the 
point 
where stuff is working to our liking.

 I too don't understand this need to sign something that you don't really know 
 what it is from some other company, just to send it to a separate device that 
 is going to do whatever it wants with it if it is signed or not.

This is not the point.  What you need to know is _where_ the firmware came 
from, 
not _what_ it does once it reach your system.  If you don't care about such 
things, just ignore the signature. :)


Petko
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/