Re: [Kernel-janitors] Fw: [CHECKER] Probable security holes in 2.6.5

[Matthew Wilcox <willy@xxxxxxxxxx>]

On Fri, Apr 16, 2004 at 02:18:27PM -0300, Luiz Fernando N. Capitulino wrote:
> Início da mensagem encaminhada:
> 
> Date: Fri, 16 Apr 2004 10:01:57 -0700
> From: Ken Ashcraft <ken@xxxxxxxxxxxx>
> To: linux-kernel@xxxxxxxxxxxxxxx
> Cc: mc@xxxxxxxxxxxxxxx
> Subject: [CHECKER] Probable security holes in 2.6.5
> 
> The error reports below are sorted roughly by their severity.  The last
> 12 of the errors are fairly minor because they are either protected by a
> capable() check or the scalar is only 8 bits.  I also consider it to be
> a minor error to pass an unchecked value to kmalloc().  I realize that
> kmalloc() will fail when asked for more than 128k, but it may not be
> appropriate to allow the user to allocate that much memory.  All of
> these minor errors will be marked by [MINOR] and/or [CAPABLE] tags in
> the error report.
> 
> ---------------------------------------------------------
> [BUG]
> /home/kash/linux/linux-2.6.5/drivers/net/e1000/e1000_ethtool.c:1494:e1000_ethtool_ioctl:
>  ERROR:TAINT: 1487:1494:Passing unbounded user value "(regs).len" as arg 2 to 
> function "copy_to_user", which uses it unsafely in model 
> [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)] 
> [SINK_MODEL=(lib,copy_to_user,user,trustingsink)]    [PATH=] 
>       }
>       case ETHTOOL_GREGS: {
>               struct ethtool_regs regs = {ETHTOOL_GREGS};
>               uint32_t regs_buff[E1000_REGS_LEN];
> 
> Start --->
>               if(copy_from_user(&regs, addr, sizeof(regs)))
>                       return -EFAULT;
>               e1000_ethtool_gregs(adapter, &regs, regs_buff);
>               if(copy_to_user(addr, &regs, sizeof(regs)))
>                       return -EFAULT;
> 
>               addr += offsetof(struct ethtool_regs, data);
> Error --->
>               if(copy_to_user(addr, regs_buff, regs.len))
>                       return -EFAULT;
> 
>               return 0;

This one is probably best fixed by converting the e1000 driver to use
ethtool_ops which does all the user-access stuff in generic code and
gets looked at by dozens more people than a driver does.  cc'ing the
e1000 people on this in case they want to do it themselves.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain