Project

General

Profile

Actions

Bug #14852

closed

Enable smatch 'sizeof' checks in the kernel

Added by Andy Fiddaman 5 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

Description

A recent draft change introduced a data corruption problem by (through a series of macros) calling sizeof(sizeof(type)) and I was following up to see why smatch did not catch this.

It turns out that smatch can detect this but that all sizeof checking is turned off in usr/src/uts due to problems arising from the use of the _IOR(), _IOW() and _IOWR() macros with a plain 0 as the third argument.

This looks like a bug in some of the definitions within sys/sockio.h which should be using the N variants of these macros.

We should fix the definitions, and re-enable sizeof checking for the kernel build.

Actions #1

Updated by Electric Monk 5 months ago

  • Gerrit CR set to 2262
Actions #2

Updated by Andy Fiddaman 5 months ago

  • Subject changed from _IO{R,W,WR}[N] confusion in sys/sockio.h to Enable smatch 'sizeof' checks in the kernel
Actions #3

Updated by Andy Fiddaman 5 months ago

When working through the ioctl macros there were a couple of places where it was hard to determine the original author's intent. For example, look at this pair:

#define        SIOCGETPROP     _IOWRN('p', 188, 0)
#define        SIOCSETPROP     _IOW('p', 189, 0)

Which of the following three is the second one supposed to be?
The argument for this particular ioctl is struct mod_ioc_prop_s which has size 0x74..

#define        SIOCSETPROP     _IOW('p', 189, int)
#define        SIOCSETPROP     _IOWN('p', 189, 0)
#define        SIOCSETPROP     _IOWN('p', 189, 4)

Either way, I realised that if I change the generated ioctl value, I break the interface and so I've
chosen to go with the first option in all cases. There are many existing definitions in this file
which pass int as the last argument.

To ensure I did not break the interface, I tested with wsdiff which shows only the expected changes:

% wsdiff proto{,.new}/root_i386-nd
usr/include/sys/sockio.h
etc/versions/build
kernel/drv/amd64/qlge
kernel/drv/amd64/oce
kernel/drv/amd64/nxge
kernel/drv/amd64/bridge
kernel/drv/amd64/acpi_drv
Actions #4

Updated by Andy Fiddaman 5 months ago

  • Description updated (diff)
Actions #5

Updated by Electric Monk 5 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit ce17336ed725d3b7fdff67bf0a0ee2b55018fec6

commit  ce17336ed725d3b7fdff67bf0a0ee2b55018fec6
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
Date:   2022-07-26T14:55:29.000Z

    14852 Enable smatch 'sizeof' checks in the kernel
    Reviewed by: Jason King <jason.brian.king+illumos@gmail.com>
    Reviewed by: Dan McDonald <danmcd@mnx.io>
    Reviewed by: Garrett D'Amore <garrett@damore.org>
    Approved by: Gordon Ross <gordon.w.ross@gmail.com>

Actions

Also available in: Atom PDF